Fix command injection on PDB download #16966

GustavoLCR · 2020-05-29T04:06:11Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description

Reported in #16945, this fixes specifically the vulnerability while downloading a PDB by avoiding calling curl through a command and using radare2's own http request API, and in case of downloads from https servers with no SSL suport from the r2 api or when extracting compressed PDBs, properly escaping the PDB name with single-quotes (double-quotes on Windows).

This includes fixes to compiling radare2 with SSL support (--with-openssl), and fixes the handshake process not being executed while connecting with SSL.

Also, now r_socket_http_get() will follow redirects automatically.

Test plan

Compile with --with-openssl
- Try to download a pdb with idpd (works)
Compile without --with-openssl
- Try to download a pdb with idpd (fails because no SSL support)
- Set %R2_CURL=1
- Try to download a pdb with idpd (works)
Download the binary from Command injection across r_sys_cmd* #16945, open and do idpd, pwned file should not get created
Do =h to run httpserver and in another terminal, connect to it.

XVilka · 2020-05-29T05:06:02Z

Seems to fail more often:
https://builds.sr.ht/~xvilka/job/221987#task-test-0

[XX] db/formats/pdb PDB downloader check
R2_NOPLUGINS=1 radare2 -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc '!!rabin2 -PP ${R2_FILE} ~PDB' bins/pdb/user32.dll
-- stdout
--- .a	2020-05-29 04:37:20.548603000 +0000
+++ .b	2020-05-29 04:37:20.548687000 +0000
@@ -1 +1 @@
-PDB "user32.pdb" download success
+PDB "user32.pdb" download failed

https://github.com/radareorg/radare2/pull/16966/checks?check_run_id=719199580#step:11:130

[XX] db/formats/pdb PDB downloader check
R2_NOPLUGINS=1 radare2 -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc '!!rabin2 -PP ${R2_FILE} ~PDB' bins/pdb/user32.dll
-- stdout

ret2libc

Do you have some reproducer steps to test the various things you fixed?

libr/bin/pdb/pdb_downloader.c

libr/socket/socket_http.c

GustavoLCR · 2020-05-31T07:51:35Z

Do you have some reproducer steps to test the various things you fixed?

@ret2libc updated pr description with test steps.

Also, newshell is failing this:
%R2_CURL=1;idpd
But this works
%R2_CURL=1 ;idpd

ret2libc · 2020-06-01T07:32:26Z

Do you have some reproducer steps to test the various things you fixed?

@ret2libc updated pr description with test steps.

Also, newshell is failing this:
%R2_CURL=1;idpd
But this works
%R2_CURL=1 ;idpd

Thanks for reporting this, it should be fixed with #16988 .

libr/socket/socket_http.c

test/db/formats/pdb

libr/util/sys.c

libr/socket/socket_http.c

libr/socket/socket.c

libr/bin/pdb/pdb_downloader.c

libr/util/str.c

libr/util/sys.c

test/db/formats/pdb

* Also follow redirects

* Fix socket being created without a protocol

* Use select() in r_socket_ready() * Fix read failing if received only protocol answer * Fix double-free

ret2libc

Added some more comments, but I think it's almost ready!

libr/socket/socket_http.c

ret2libc · 2020-06-05T08:01:01Z

libr/bin/pdb/pdb_downloader.c

@@ -191,6 +151,19 @@ void deinit_pdb_downloader(SPDBDownloader *pd) {
 	pd->download = 0;
 }

+static bool is_valid_guid(const char *guid) {


GUID are 32 chars long, aren't they? I'm not sure this function works correctly.
e.g.:

is_valid_guid('12345678901234567890123456789012') should be true is_valid_guid('123456789012345678901234567890123') should be false

I think that currently this function does not return the right values.

tested it, function works correctly

Mmm, for me it doesn't. I'm using this test, where is_valid_guid is copied-pasted from your PR.

#include <stdio.h> #include <stdbool.h> #include <ctype.h> static bool is_valid_guid(const char *guid) { if (!guid) { return false; } size_t i; for (i = 0; guid[i] && i <= 33 ; i++) { if (!isxdigit (guid[i])) { return false; } } return i == 33; } static void test_s(const char *s) { if (is_valid_guid (s)) { printf("'%s' is valid guid\n", s); } else { printf("'%s' is NOT valid guid\n", s); } } int main(int argc, char **argv) { test_s("12345678901234567890123456789012"); test_s("123456789012345678901234567890123"); return 0; }

If I run this program, it says:

'12345678901234567890123456789012' is NOT valid guid '123456789012345678901234567890123' is valid guid

but the expected output is:

'12345678901234567890123456789012' is valid guid '123456789012345678901234567890123' is NOT valid guid

Please let me know if I'm missing something.

turns out guid string has appended to it the "age", so assuming it even has a fixed length was wrong, fixed it now

mh, that looks weird, but it seems it is something a bit more deeper in the existing code. Guid are 32 chars AFAIK. Nevermind.

libr/bin/pdb/pdb_downloader.c

* Use r_socket_http_get() on UNIXs * Use WinINet API on Windows for r_socket_http_get() * Fix command injection

* Remove 'r_' and '__' from static function names

libr/socket/socket_http.c

ret2libc

Added some last minor stuff + doubts about is_valid_guid. Once is_valid_guid is fixed (or you prove me I'm doing something wrong :D) I think we can merge this. Thanks a lot for all the changes you have done!

trufae

Good work! nice to see new escaping APIs, we probably need to have the unescape_sh() one too, and have more tests for them

libr/bin/pdb/pdb_downloader.c

libr/socket/socket_http.c

libr/socket/socket.c

libr/bin/pdb/pdb_downloader.c

ret2libc

Looks good, thanks again!

* Fix r_sys_mkdirp with absolute path on Windows * Fix build with --with-openssl * Use RBuffer in r_socket_http_answer() * r_socket_http_answer: Fix read for big responses * Implement r_str_escape_sh() * Cleanup r_socket_connect() on Windows * Fix socket being created without a protocol * Fix socket connect with SSL ##socket * Use select() in r_socket_ready() * Fix read failing if received only protocol answer * Fix double-free * r_socket_http_get: Fail if req. SSL with no support * Follow redirects in r_socket_http_answer() * Fix r_socket_http_get result length with R2_CURL=1 * Also follow redirects * Avoid using curl for downloading PDBs * Use r_socket_http_get() on UNIXs * Use WinINet API on Windows for r_socket_http_get() * Fix command injection * Fix r_sys_cmd_str_full output for binary data * Validate GUID on PDB download * Pass depth to socket_http_get_recursive() * Remove 'r_' and '__' from static function names * Fix is_valid_guid * Fix for comments

GustavoLCR requested review from radare, ret2libc, thestr4ng3r, XVilka and yossizap as code owners May 29, 2020 04:06

GustavoLCR changed the title ~~Fix pdb cmd inj~~ Fix command injection on PDB download May 29, 2020

github-actions bot added the RBin label May 29, 2020

ret2libc reviewed May 29, 2020

View reviewed changes

libr/bin/pdb/pdb_downloader.c Show resolved Hide resolved

libr/socket/socket_http.c Outdated Show resolved Hide resolved

GustavoLCR force-pushed the fix-pdb-cmd-inj branch 7 times, most recently from 933f615 to 2184566 Compare May 31, 2020 07:48

GustavoLCR force-pushed the fix-pdb-cmd-inj branch 4 times, most recently from 588ce77 to 1bb5d69 Compare May 31, 2020 22:00

XVilka approved these changes Jun 1, 2020

View reviewed changes

ret2libc mentioned this pull request Jun 1, 2020

Fix ; at the end of the value of eq_sep_args #16988

Merged

4 tasks

ret2libc suggested changes Jun 1, 2020

View reviewed changes

GustavoLCR force-pushed the fix-pdb-cmd-inj branch from 1bb5d69 to 930b445 Compare June 2, 2020 00:59

github-actions bot added the API New API requests, changes, removal label Jun 2, 2020

GustavoLCR force-pushed the fix-pdb-cmd-inj branch 2 times, most recently from 8840369 to c4e2872 Compare June 2, 2020 03:57

ret2libc reviewed Jun 3, 2020

View reviewed changes

GustavoLCR added 9 commits June 4, 2020 16:59

Fix build with --with-openssl

f37ea3c

Use RBuffer in r_socket_http_answer()

907ce7f

r_socket_http_answer: Fix read for big responses

aa638a5

Implement r_str_escape_sh()

6024294

Follow redirects in r_socket_http_answer()

70634b2

Fix r_socket_http_get result length with R2_CURL=1

88eb4e1

* Also follow redirects

Cleanup r_socket_connect() on Windows

94067c4

* Fix socket being created without a protocol

Fix socket connect with SSL ##socket

d3a2e0d

* Use select() in r_socket_ready() * Fix read failing if received only protocol answer * Fix double-free

r_socket_http_get: Fail if req. SSL with no support

a6b80ba

GustavoLCR force-pushed the fix-pdb-cmd-inj branch from c4e2872 to cf7bf20 Compare June 4, 2020 20:16

github-actions bot added the r2r Regression tests label Jun 4, 2020

ret2libc suggested changes Jun 5, 2020

View reviewed changes

GustavoLCR added 4 commits June 6, 2020 23:52

Avoid using curl for downloading PDBs

cb65af1

* Use r_socket_http_get() on UNIXs * Use WinINet API on Windows for r_socket_http_get() * Fix command injection

Fix r_sys_cmd_str_full output for binary data

6d0a11a

Validate GUID on PDB download

cdeb3c6

Pass depth to socket_http_get_recursive()

a6221f8

* Remove 'r_' and '__' from static function names

GustavoLCR force-pushed the fix-pdb-cmd-inj branch from 01f3c5a to a6221f8 Compare June 7, 2020 02:53

XVilka approved these changes Jun 8, 2020

View reviewed changes

ret2libc reviewed Jun 8, 2020

View reviewed changes

libr/socket/socket_http.c Outdated Show resolved Hide resolved

ret2libc reviewed Jun 8, 2020

View reviewed changes

libr/socket/socket_http.c Outdated Show resolved Hide resolved

ret2libc reviewed Jun 8, 2020

View reviewed changes

libr/socket/socket_http.c Outdated Show resolved Hide resolved

ret2libc suggested changes Jun 8, 2020

View reviewed changes

trufae requested changes Jun 8, 2020

View reviewed changes

GustavoLCR added 2 commits June 9, 2020 14:49

Fix is_valid_guid

f6d225d

Fix for comments

9593150

ret2libc approved these changes Jun 10, 2020

View reviewed changes

trufae approved these changes Jun 10, 2020

View reviewed changes

ret2libc merged commit 04edfa8 into radareorg:master Jun 10, 2020

GustavoLCR deleted the fix-pdb-cmd-inj branch October 14, 2020 03:52

Uh oh!

Fix command injection on PDB download #16966

Fix command injection on PDB download #16966

Uh oh!

Conversation

GustavoLCR commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XVilka commented May 29, 2020

Uh oh!

ret2libc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GustavoLCR commented May 31, 2020

Uh oh!

ret2libc commented Jun 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ret2libc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ret2libc Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

GustavoLCR Jun 7, 2020

Choose a reason for hiding this comment

Uh oh!

ret2libc Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

GustavoLCR Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ret2libc Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ret2libc left a comment

Choose a reason for hiding this comment

Uh oh!

trufae left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ret2libc left a comment

Choose a reason for hiding this comment

Uh oh!

GustavoLCR commented May 29, 2020 •

edited

Loading