JSON output not properly escaped in /j

[ghost]

Same problem as #7211 . When returning a hit, if a backslash is present in the data section it is not escaped.

[0x0040b240]> ?V
1.4.0-git aka 1.3.0-383-g97cb382 commit 14482
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":2,"data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

binary used : any teamspeak server binary, here 3.0.13.6 http://dl.4players.de/ts/releases/3.0.13.6/teamspeak3-server_linux_amd64-3.0.13.6.tar.bz2

[Maijin]

Look at the commit then https://github.com/radare/radare2/commit/04ad4dcf413c87ab3c27b534c1ef026c44b63bad and fix it !

[ghost]

I'm just trying to help by reporting an issue. I don't believe I have the required skills nor the knowledge on how radare's codebase is maintained to successfully fix it.

[Maijin]

b; -> r_str_escape (b);

[ghost]

Not fixed.

[0x0040b240]> ?V
1.4.0-git aka 1.3.0-392-geca1576 commit 14492
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":0,"data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

Or did I miss something ?

[radare]

Wtf this is not a escape problem. How to reproduce?

On 16 Apr 2017, at 12:48, ehthe notifications@github.com wrote:

Not fixed.

[0x0040b240]> ?V 1.4.0-git aka 1.3.0-392-geca1576 commit 14492 [0x0040b240]> /j 2622454d494 [{"offset": 8510287,"id:":0,"data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

Or did I miss something ?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

Steps to reproduce :

1. Download radare (git clone, sys/install.sh)
2. open ts3server ( http://dl.4players.de/ts/releases/3.0.13.6/teamspeak3-server_linux_amd64-3.0.13.6.tar.bz2 ) : radare2 ts3server
3. /j 2622454d494

To be entirely precise the backslashes are actually here. In the string aforementioned, in the binary. So any binary that has some Windows path in it should reproduce the issue.

[radare]

I dont think you wanna search this string. I guess you want /xj

On 16 Apr 2017, at 13:07, ehthe notifications@github.com wrote:

Step to reproduce :

Download radare (git clone, sys/install.sh) open ts3server ( http://dl.4players.de/ts/releases/3.0.13.6/teamspeak3-server_linux_amd64-3.0.13.6.tar.bz2 ) : radare2 ts3server /j 2622454d494 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[alvarofe]

[0x0040b240]> /j 2622454d494|json_pp
illegal backslash escape sequence in string, at character offset 65 (before "\\windows\\syste"}]\n...") at /usr/local/bin/json_pp line 45.

[0x0040b240]> /j 2622454d494~{}
[
  {
    "offset": 8510287,
    "id:": 2,
    "data": "a7e702c417a1ad4f2622454d494c:\windows\syste"
  }
]

[ghost]

Well I did use /xj at first but when you're looking for strings it's a bit more user friendly to use /j :) That's when I found out about that json esaping bug.

[radare]

👍

On 16 Apr 2017, at 20:27, ehthe notifications@github.com wrote:

Well I did use /xj at first but when you're looking for strings it's a bit more user friendly to use /j :) That's when I found out about that json esaping bug.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[alvarofe]

it should be fixed now besides I added a test in r2r. Test again please and reopen it again in case I didn't fix it.

[ghost]

It is now working. Thanks you very much.

[dequis]

https://github.com/radare/radare2-regressions/commit/cfa30d06d43ab017404f3ac25839ccec5f3cb052

-EXPECT='[{"offset": 0,"id:":0,"data":"foo\u0000"}]
+EXPECT='[{"offset": 0,"id:":0,"data":"foo\\u0000"}]

That's not valid output. It's double escaping the backslash of \u0000

[dequis]

Not fixed.

[0x0040b240]> ?V 1.4.0-git aka 1.3.0-392-geca1576 commit 14492

It wasn't fixed because eca1576 is the commit where pancake reverted the fix. I don't really understand why he did that.

[radare]

If you escape the \u then you are having the wrong data in the value. Nodejs parses this fine. And its part of the json standard

On 17 Apr 2017, at 01:02, dx notifications@github.com wrote:

Not fixed.

[0x0040b240]> ?V 1.4.0-git aka 1.3.0-392-geca1576 commit 14492

It wasn't fixed because eca1576 is the commit where pancake reverted the fix. I don't really understand why he did that.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[dequis]

Let me put this differently with a bunch of words so that there are no ambiguities. Given these commits:

• 89fea59cc (origin/master, master) Fix oob read in radare2.c when -R
• 5f1efc9d6 (master~1) Fix #7282 - escape data in /j output
• 8570f4fc5 (master~2) fixed r_types.h use in wasm (#7292)
• eca157603 (master~3) Revert "r_str_utf16_encode: escape backslash characters too (#7288)" (#7289)
• 1c6a31382 (master~4) r_str_utf16_encode: escape backslash characters too (#7288)
• 04ff4a519 (master~5) Fix #6428 - Honor scr.color in ad command

This is the behavior of 04ff4a519 (master~5), the oldest one before any fixes. The null is correctly escaped (by r_str_utf16_encode) but the backslash isn't.

[0x00000000]> wx 005c
[0x00000000]> /j \x00\\
[{"offset": 0,"id:":0,"data":"\u0000\"}]

This is the behavior of 1c6a31382 (master~4, reverted by master~3), which modifies r_str_utf16_encode to escape backslashes instead of just nonprintable characters, so null is turned into \u0000 and the backslash is turned into \\

[dx@bolsa test/radare/radare2]$ r2 -
[0x00000000]> wx 005c
[0x00000000]> /j \x00\\
[{"offset": 0,"id:":0,"data":"\u0000\\"}]

This is the current behavior, introduced in 5f1efc9d6 (master~1). The null is doubly escaped incorrectly, because it was handled by r_str_utf16_encode first (turning null into \u0000) and then both backslashes are escaped again by r_str_escape

[0x00000000]> wx 005c
[0x00000000]> /j \x00\\
[{"offset": 0,"id:":0,"data":"\\u0000\\"}]

The radare2-regressions commit I linked above (https://github.com/radare/radare2-regressions/commit/cfa30d06d43ab017404f3ac25839ccec5f3cb052) modified the tests so the expected result for escaping a null \\u0000 with an incorrect double backslash instead the previous value of \u0000. That makes it pass with the current wrong behavior introduced by 5f1efc9d6.

[radare]

Confirm its fixed now in master. Your path was not solving the problem

On 17 Apr 2017, at 07:30, dx notifications@github.com wrote:

Let me put this differently with a bunch of words so that there are no ambiguities. Given these commits:

89fea59 (origin/master, master) Fix oob read in radare2.c when -R 5f1efc9 (master~1) Fix #7282 - escape data in /j output 8570f4f (master~2) fixed r_types.h use in wasm (#7292) eca1576 (master~3) Revert "r_str_utf16_encode: escape backslash characters too (#7288)" (#7289) 1c6a313 (master~4) r_str_utf16_encode: escape backslash characters too (#7288) 04ff4a5 (master~5) Fix #6428 - Honor scr.color in ad command This is the behavior of 04ff4a5 (master~5), the oldest one before any fixes. The null is correctly escaped (by r_str_utf16_encode) but the backslash isn't.

[0x00000000]> wx 005c [0x00000000]> /j \x00\ [{"offset": 0,"id:":0,"data":"\u0000\"}] This is the behavior of 1c6a313 (master~4, reverted by master~3), which modifies r_str_utf16_encode to escape backslashes instead of just nonprintable characters, so null is turned into \u0000 and the backslash is turned into \

[dx@bolsa test/radare/radare2]$ r2 - [0x00000000]> wx 005c [0x00000000]> /j \x00\ [{"offset": 0,"id:":0,"data":"\u0000\"}] This is the current behavior, introduced in 5f1efc9 (master~1). The null is doubly escaped incorrectly, because it was handled by r_str_utf16_encode first (turning null into \u0000) and then both backslashes are escaped again by r_str_escape

[0x00000000]> wx 005c [0x00000000]> /j \x00\ [{"offset": 0,"id:":0,"data":"\u0000\"}] The radare2-regressions commit I linked above (radare/radare2-regressions@cfa30d0) modified the tests so the expected result for escaping a null \u0000 with an incorrect double backslash instead the previous value of \u0000. That makes it pass with the current wrong behavior introduced by 5f1efc9.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[dequis]

Your path was not solving the problem

Can you explain what was wrong with it? I thought it was clear from above that the problem is fixed with my patch. It didn't work for OP before only because you reverted the fix before they tested.

8fd10e11aae8f8f0e4112883936a77826a4b65eb adds a hack to r_str_escape so that backslashes are not escaped if they are followed by a 'u' character. Which means I can add a 'u' character after a backslash in my input string and do this:

[0x00000000]> wx 005c75
[0x00000000]> /j \x00\\
[{"offset": 0,"id:":1,"type":"string","data":"\u0000\u"}]

Which is invalid JSON

If you had just merged my PR directly you'd have this behavior:

[0x00000000]> wx 005c75
[0x00000000]> /j \x00\\
[{"offset": 0,"id:":0,"data":"\u0000\\u"}]

[radare]

with your PR i had “\u0000\ smb://u0000//” maybe i tested it wrong, but \u smb://u is not what i was expecting. can you rebase and add more tests? at least now its failing in less cases and i have fixed the /xj which was wrong too.

thanks

On 17 Apr 2017, at 16:36, dx notifications@github.com wrote:

[ghost]

Unfortunately it seems the issue has resurfaced :/

[0x0040b240]> ?V
1.5.0-git aka 1.4.0-313-g49332a26b commit 14862
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

[radare]

Missing tests?

On 25 May 2017, at 23:29, ehthe notifications@github.com wrote:

Unfortunately it seems the issue has resurfaced :/

[0x0040b240]> ?V 1.5.0-git aka 1.4.0-313-g49332a26b commit 14862 [0x0040b240]> /j 2622454d494 [{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[radare]

Are u using r2-1.3? Really?

On 25 May 2017, at 23:29, ehthe notifications@github.com wrote:

Unfortunately it seems the issue has resurfaced :/

[0x0040b240]> ?V 1.5.0-git aka 1.4.0-313-g49332a26b commit 14862 [0x0040b240]> /j 2622454d494 [{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

Well no, I'm using 1.4 as shown in https://github.com/radare/radare2/issues/7282#issuecomment-304130075

[radare]

1.4 was released before this bug was fixed and you are not usin 1.4 acording to the output you pasted

On 26 May 2017, at 14:05, ehthe notifications@github.com wrote:

Well no, I'm using 1.4 as shown in #7282 (comment)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

I'm pasting my current output again, you can see that I'm using 1.5.0-git

[0x0040b240]> ?V
1.5.0-git aka 1.4.0-313-g49332a26b commit 14862
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\windows\syste"}]

I use the instructions at http://radare.org/r/down.html to get the newest version. Maybe I've missed something ?

[ghost]

Works again :D

[0x0040b240]> ?V
1.5.0-git aka 1.4.0-322-g0b84978a8 commit 14873
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\\windows\\syste"}]

[Maijin]

there was already a test but for whatever reason it was put on BROKEN=1 state. I have removed that + added another one

[radare]

Thanks. Maybe someone can take care of this and send a pr fixing those cases

On 27 May 2017, at 12:40, Maijin notifications@github.com wrote:

there was already a test but for whatever reason it was put on BROKEN=1 state. I have removed that + added another one

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[Maijin]

? Works now

[ghost]

Working :D

[0x0040b240]> ?V
1.5.0-git aka 1.4.0-326-g5d2b961a7 commit 14877
[0x0040b240]> /j 2622454d494
[{"offset": 8510287,"id:":0,"type":"string","data":"a7e702c417a1ad4f2622454d494c:\\windows\\syste"}]
[0x0040b240]>