Enforce that Rust code is proxy-safe (doesn't call directly into libc networking functions)

NEW
Assigned to

Status

P2
normal
2 years ago
8 hours ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Blocks: 2 bugs)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix)

Details

(Whiteboard: [tor 21862])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The rust code in 52 might have been proxy unsafe, so Tor turned it off. We should make sure it is proxy safe.

https://trac.torproject.org/projects/tor/ticket/21862
Somehow related: bug 1367932 comment 1.
Blocks: 1260929
Priority: -- → P2
The getaddrinfo call is from the ToSocketAddrs impl, which is a trait impl in the url crate not used by the Rust integration.

As far as I can tell, none of the ongoing Rust projects should involve any getaddrinfo calls.
Is there any way we can reliably enforce that?
What do we do for C++ code right now?

I suspect the correct linker script / link args would work. I tried compiling an empty Rust library with LTO (rustc test.rs -Clto --crate-type=staticlib), however the library still had an unlinked getaddrinfo symbol. There should be some combo of link args and nm/objdump/readelf calls that let us differentiate between a staticlib that uses a symbol and one that doesn't. I'm not sure what it is.
I don't believe we have anything for C++ code, because historically we haven't imported arbitrary C++ code that can do networking. :) I guess the best thing to do here would be to add stub functions for libc networking functions that we don't want Rust code calling directly, and have them proxy everything to Necko?

It sounds like as-filed this bug is INVALID, because the way we're using the rust-url crate does not cause it to do DNS lookups with getaddrinfo, but we don't have any backstops that would actually prevent that, so maybe we can just morph this bug to cover that.

glandium: does my vague proposal sound plausible?
Summary: Rust Code may not obey proxy safety → Enforce that Rust code is proxy-safe (doesn't call directly into libc networking functions)
I would rather have stub functions that just abort.

We'd have to force Rust to link to those in such a way that the final XUL link step isn't polluted. I'm not sure how that can be done, perhaps a linker script?
I assume that if we linked in a Rust crate that provided the right symbols it might work, but we'd have to ensure that they did not wind up as public symbols to which the other stuff in XUL would work. I'm also not sure what extern linkage actually does in Rust, so it's possible this would not work.
We create a regular C-like staticlib; we could link those symbols in first and create a different staticlib, and then link it into XUL?
This bug is showing up in some Firefox 57 tracking dashboard because it was changed recently. I don't think it has anything to do with Firefox 57, so I'm setting the tracking flag so it doesn't show up. If anyone disagrees, please change the flag!
status-firefox57: --- → wontfix
(Assignee)

Comment 10

a year ago
Hey Georg, I'm flagging you in this to help us figure out what/how we should proceed.
Flags: needinfo?(gk)

Comment 11

a year ago
Not sure what exactly I should help with but comments 5-8 had some ideas about how to solve this bug. I am not familiar enough with Rust/linking internals to evaluate that one but, yes, we are very interested in a solution that avoids us removing random network related Rust APIs just because we can't be sure that there is no way they can bypass the proxy settings in the browser.
Flags: needinfo?(gk)
Whiteboard: [tor] → [tor 21862]

Updated

11 months ago
Product: Core → Firefox Build System
(Assignee)

Comment 12

15 days ago

We talked about this a bit today and established at least the next step forward:

<@ted> we could definitely use the same techniques we use to replace malloc and abort to replace stuff like socket / connect, but that'd apply to all of libxul
<@ted> so we'd have to ensure that we made the existing networking code in Firefox work with it first
<@ted> i'm not sure there's going to be a sensible way to enforce this for just rust code
<froydnj> could we just lint the rust staticlib(s) for things we don't want to see? won't solve dlopen issues, but would be a good first step?
<tjr> So let me speculate wildly about things that I don't really know how they work..... When we're compiling the rust code; we're making object files that will eventually get merged (linked?) into the resulting library (xul).
<tjr> Those object files must have symbols they need from other libraries. Can we look for particular symbols that are scary and abort or something?
<@ted> sure
<tjr> (This kind of sounds like what froydnj just said...)
<@ted> yeah :)
<@ted> i'm not sure if that wouldn't produce false positives right now, though, since we might be compiling crates that have networking code that we're not using?
<froydnj> tjr: great minds think alike! ;)
<tjr> Hm. And modifying third party crates is quite a maintenance burden.... Would it be crazy to suggest modifying the object file (or preprocess the crate) to swap those functions out to call stub_socket or whatever; and provide implementations that runtime abort?
<@ted> those both sound fragile
<tjr> Well; as a starting point I guess we can see how many occurances of the third party crate problem we have....
<tjr> would you be able to point me where to start putting some object file investigation work?
<froydnj> my gut feeling is that there shouldn't be too many false positives and if there are false positives, that probably means the crate functionality needs to be split up
<@ted> maybe!
<froydnj> tjr: you probably want to put a check into the rule for rust target libraries: https://searchfox.org/mozilla-central/source/config/rules.mk#1038-1040
<froydnj> tjr: and the check should look something like $(TOOLCHAIN_PREFIX)readelf -sW | awk '$7 == "UND" { print $8 }' |egrep -q 'socket|connect|accept|...' 2>/dev/null && echo 'TEST-UNEXPECTED-FAIL | check_networking' || true
<froydnj> readelf -sW $some_magic_rust_library
<tjr> Thanks!
<@ted> we've got mio and net2 vendored, i don't know if either of those actually wind up in gkrust
<froydnj> mio does on linux, I think
<froydnj> for remoting audio

(Assignee)

Comment 13

5 days ago

So at this point; I have a patch that will search through the built rust static libraries[0] for exported symbols we care about by running

readelf -sW /home/tom/Documents/moz/mozilla-unified-2/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/release/libgkrust.a | awk '$7 == "UND" { print $8 }' | egrep 'socket|connect|accept'

When built without --enable-release, readelf -sW will produce a gigantic file (85MB) with many of the 3 initial symbols.

When built with --enable-release readelf -sW output is 3.2M and does not contain any of those symbols.

From a previous discussion on #build[1], I was thinking I might need to perform dead code elimination on the rust static libraries by completing some sort of partial link process with libxul to remove what was needed; but now it looks like that is not necessary; and performing this symbol search when enable-release is enabled is a path forward...

[0] obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/[debug,release]/[libgkrust.a, libjsrust.a]
[1] https://mozilla.logbot.info/build/20190116#c15838788

(Assignee)

Comment 14

2 days ago

Created attachment 9037824 [details] [diff] [review]
Bug 1376621 - Search for networking symbols in rust code and fail if they are found

As a checkpoint, I got this working (and passing on Linux) with the attached patch. After talking with glandium; I'm going to refactor this to a check_binary check. And then I'll start trying to be more exhaustive with the networking functions we look for.

Assignee: nobody → tom
(Assignee)

Comment 15

14 hours ago

Created attachment 9038023 [details] [diff] [review]
Bug 1376621 - Search for networking symbols in rust code and fail if they are found

Alright; here's a first pass to get some feedback on the changes to check_binary.py

Attachment #9037824 - Attachment is obsolete: true
Attachment #9038023 - Flags: review?(mh+mozilla)
(Assignee)

Comment 16

13 hours ago

Hm. This does not work for anything but Linux. I am investigating finding the correct readelf programs/command for OSX, Windows, and mingw-clang...

Comment on attachment 9038023 [details] [diff] [review]
Bug 1376621 - Search for networking symbols in rust code and fail if they are found

Review of attachment 9038023 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/action/check_binary.py
@@ +311,5 @@
>  
>  
> +def check_networking(target, binary):
> +    retcode = 0
> +    networking_functions = ["socket", "connct", "accept"]

Missing a e in connect.

@@ +319,5 @@
> +    try:
> +        for sym in at_least_one(iter_readelf_symbols(target, binary)):
> +            if sym['index'] == 'UND' and sym['name'] in networking_functions:
> +                bad_occurances_count += 1
> +                bad_occurances_names.add(sym['name'])

Keeping a separate count doesn't sound too useful. len(bad_occurances_names) will have the same value in most cases, and even if it doesn't have the same value, it's not all that important.

@@ +326,5 @@
> +
> +    basename = os.path.basename(binary)
> +    if bad_occurances_count:
> +        s = 'TEST-UNEXPECTED-FAIL | check_networking | {} | Identified {} ' + \
> +            'networking function(s) being imported in the rust static libraries ({})'

library

@@ +328,5 @@
> +    if bad_occurances_count:
> +        s = 'TEST-UNEXPECTED-FAIL | check_networking | {} | Identified {} ' + \
> +            'networking function(s) being imported in the rust static libraries ({})'
> +        print(s.format(basename, bad_occurances_count,
> +            ",".join(x for x in bad_occurances_names)),

join(bad_occurances_names) does the same, but sorted() would be even better.

Also, it's occurrences :)

@@ +399,5 @@
>          return checks(HOST, options.binary)
>      elif options.target:
>          return checks(TARGET, options.binary)
> +    elif options.networking:
> +        return check_networking(TARGET, options.binary)

Considering there _are_ host and target rust libraries, and that this only works for target rust libraries, I'd rather have one of --host/--target still be required, and --networking --host be an explicit error.
Attachment #9038023 - Flags: review?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.