Page MenuHomeFreeBSD

Added SOCKS5 support to libfetch
Needs ReviewPublic

Authored by farhan_farhan.codes on Jan 21 2019, 12:26 AM.

Details

Reviewers
des
0mp
thj
Group Reviewers
manpages
Summary

This change adds SOCKS5 support to the library fetch(3) and updates the man page.

Details: Within the fetch_connect() function, fetch(3) checks if the SOCKS5_PROXY environment variable is set. If so, it connects to this host rather than the end-host. It then initializes the SOCKS5 connection in accordance with RFC 1928 and returns the resulting conn_t (file descriptor) for usage by the regular FTP/HTTP handlers.

Design Decision: This change defaults all DNS resolutions through the proxy by sending all IPs as hostnames. Going forward, another feature might be to create another environmental variable to toggle resolutions through the proxy or not..

For Special Review: There is error reporting in fetch_socks5_init to stderr. I could not figure out the proper method to set fetchLastErrString to a SOCKS5-specific error message, which is consumed by end-clients like fetch(1). If you know how, we can include SOCKS5-specific error messages. This might be useful so that fetch(1) will report a proper error.

Test Plan

Set the SOCKS5_PROXY environment variable in any of the formats:

SOCKS5_PROXY=proxy.example.com
SOCKS5_PROXY=proxy.example.com:1080
SOCKS5_PROXY=192.0.2.0
SOCKS5_PROXY=198.51.100.0:1080
SOCKS5_PROXY=[2001:db8::1]
SOCKS5_PROXY=[2001:db8::2]:1080

Then perform a request with fetch(1).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

If in the future we create an environmental variable toggle switch for DNS over the proxy vs locally, we should make sure not to use getaddrinfo() or fetch_resolve() of the destination host when DNS-over-SOCKS5 is set. This could result in a unintended DNS leaks and compromise privacy.

From glancing at this patch, there's a few code security-related issues that need to be worked out.

lib/libfetch/common.c
351

Should this size be macro-ized?

359

Magic values should be macro-ized.

389

ptr is already set to buf.

395

Since you're doing pointer arithmetic, strlen(host) needs to be validated to be 256 characters or less. As you modify where ptr points to, you need to ensure ptr is still within bounds prior to dereferencing it.

426

No need for the breaks after goto.

484

Need to check for NULL return value.

495

Need to check for NULL return value.

506

Need to check for NULL return value.

510

Need to check for NULL return value.

0mp requested changes to this revision.Jan 22 2019, 10:33 AM
0mp added a subscriber: 0mp.

Cool!

BTW, you may take a look at style.mdoc(5).

lib/libfetch/fetch.3
29

Remember to bump the date.

672

Please lint your changes using mandoc -Tlint fetch.3 and igor fetch.3 (https://www.freshports.org/textproc/igor).

674

Typo:

If no port

676

.Ev HTTP_PROXY

736

.Ev SOCKS5_PROXY

740

I'd use something like .Bd -literal -offset indent instead of multiple .Dl. See NO_PROXY above for example.

This revision now requires changes to proceed.Jan 22 2019, 10:33 AM

I'm so sorry, I missed your comment from months ago!

farhan_farhan.codes marked 12 inline comments as done.

Updated per @lattera-gmail.com and @0mp comments.

0mp requested changes to this revision.Jun 19 2019, 8:41 AM

Only some minor issues left in the manpage :)

lib/libfetch/fetch.3
676
This setting will supercede a connection to an
. Ev HTTP_PROXY .
This revision now requires changes to proceed.Jun 19 2019, 8:41 AM

Updated the man page + some macros

0mp accepted this revision.Jun 20 2019, 1:45 AM

ok from manpages

This revision is now accepted and ready to land.Jun 20 2019, 1:45 AM

Any updates on this?

thj added a reviewer: thj.Aug 8 2019, 8:07 AM

Hi all. I see that this was changed to "Accepted" a while ago, but there has not been any movement on it.

kevans added a subscriber: kevans.Aug 19 2019, 9:29 PM
kevans added inline comments.
lib/libfetch/common.c
383

Move this else up to the line above it; if we could get rid of the magic number here, that'd be great.

469

This should read: (socks5env = getenv("SOCKS5_PROXY")) == NULL || *socks5env == '\0' (prefer explicitly comparisons as per style(9))

472

I'll leave this note here once; this function returns 0, 1, or 2 -- the difference between 1 and 2 doesn't seem to be significant from a callers' perspective, and I'm not sure this matches the prevailing style for libfetch which tends to return 0 for success, -1 for error.

477

Binary operators should have spaces around them (strlen(socks5env) - 1)

483

Move this a line up, } else {

485

ext == NULL

490

See above w.r.t. binary operators and spaces around them.

491

See above w.r.t. binary operators and spaces around them; ext and socks5env should also likely be unparenthesized here.

494

See above w.r.t. binary operators and spaces around them.

Also, why strtoimax here and below? You're given an int to work with here, so this should likely be something more like:

long portconv;

portconv = strtol(...);
if (portconv <= 0 || portconv > INT_MAX)
    ... error ...
*port = portconv;

strtoimax and strtol are effectively the same, just with different min/max limits -- you might as well use the smallest set of limits larger than what you can handle, even if efficiency isn't all that much of a concern here.

501

else should move to the line above it

503

ext == NULL

507

Moved to the line above it

508

See above w.r.t. binary operators and spaces around them.

511

See above w.r.t. binary operators and spaces around them.

533

This should move down to the line with other int declarations, sorted alphabetically there.

544

sockshost == NULL

575

Moved up to the line above

580

Braces around this single-statement block should match the prevailing style elsewhere in the file for single-statement blocks; it looks like the braces should be dropped here.

599

Added line should go away

farhan_farhan.codes marked 17 inline comments as done.

Updates from @kevans comments

This revision now requires review to proceed.Aug 20 2019, 2:05 AM
farhan_farhan.codes marked 2 inline comments as done.Aug 20 2019, 2:06 AM

I believe I got all updates, except one. I can go ahead and make that change ASAP.

lib/libfetch/common.c
580

This one seems correct, no?

kevans added inline comments.Aug 20 2019, 1:12 PM
lib/libfetch/common.c
580

Correct, but stylistically inconsistent with the rest of the file- take a look ,for instance, at lines 556/557.

Minor style(9) change

Updated!

lib/libfetch/common.c
580

oh, I misunderstood your original comment. My mistake. Removing now.

Rebased against current head

Any updates on this, @des and @thj ? :)

For Special Review: There is error reporting in fetch_socks5_init to stderr. I could not figure out the proper method to set fetchLastErrString to a SOCKS5-specific error message, which is consumed by end-clients like fetch(1). If you know how, we can include SOCKS5-specific error messages. This might be useful so that fetch(1) will report a proper error.

This is actually pretty easy, from the looks of it- refer to common.c:67 (netdb_errlist) -- you'll want to define another structure like this for socks5_errlist mapping internal error codes (you'll need to create these) to FETCH_* error codes (see fetch.h:73) and error strings. The common theme is then to define socks5_seterr() like the others in common.h and use that with your error codes.

The downside of this scheme is that it doesn't let you include additional formatting specifiers, but those details are perhaps better left to a verbose option kind of thing.