LogAccessHttp init strlen to 0, not -1 (master) #2102

a-canary · 2017-06-08T20:21:11Z

Code was using -1 string length to signify that had not been validated yet. This caused marshal_mem to allocate 0 bytes, below the INK_MIN_ALIGN, and write to unallocated mem.
Now code initializes all string length vars to 0, and upon failure to validate, the string ptr is set to INVALID_STR to prevent multiple validataions attempts.

Fixes YTSATS-1240

jpeach

Please fix the git user.name:

From: Unknown <acanary@yahoo-inc.com>

zwoop · 2017-06-08T20:27:22Z

This has to go on the 7.1.x branch too ?

bryancall · 2017-06-08T20:32:27Z

[approve ci]

a-canary · 2017-06-08T20:47:47Z

Yahoo doesn't need it ported to 7.1, given that is only a rare logging error that only we are seeing. I am looking into back porting to 5.3, because that is where the issue was discovered by our analysis team.

unknown user.name... I'm a git newb.

jpeach · 2017-06-08T20:49:19Z

On Jun 8, 2017, at 1:47 PM, a-canary ***@***.***> wrote: Yahoo doesn't need it ported to 7.1, given that is only a rare logging error that only we are seeing. I am looking into back porting to 5.3, because that is where the issue was discovered by our analysis team. unknown user.name... I'm a git newb.

Take a look at https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup

a-canary · 2017-06-08T20:55:37Z

User name fixed

leighklotz · 2017-06-08T21:01:33Z

Given that this is a write to unallocated memory, is it a potential security issue that should be fixed in latest version as well?

a-canary · 2017-06-08T21:20:06Z

It is not a security issue because it is contained to the log buffer memory, and output.

Specifically this case it is writing past the end of it's allocated log buffer by 1 byte. It would write "-", a second thread would allocate and write that same address, then the first thread would flush the log buffer to a file, and the "-" is replaced with a random character.

bryancall · 2017-06-08T21:20:44Z

proxy/logging/LogField.cc

  for (LogField *f = first(); f; f = next(f)) {
    if (f->type() != LogField::sINT) {
-      bytes += f->marshal_len(lad);
+      const int len = f->marshal_len(lad);


Wouldn't adding a check on len > 0 here and not add it to bytes also work? This way you wouldn't have to set m_cache_lookup_url_canon_str or have INVALID_STR.

The core of the problem was LogAccessHttp.cc:ln487,
len = round_strlen(m_client_req_unmapped_url_host_len + 1); // +1 for eos
When m_client_req_unmapped_url_host_len is -1, len is set to 0.
And strlen of -1 is kind of gross way to indicate that can't resolve the host name. In the future we would like to change all these to StringView, so this nudges us in that direction.

This assert is just to ensure no other code paths where having the same issue.

I am not seeing that in the code on master round_strlen(m_client_req_unmapped_url_host_len + 1);. What line are you referring to?

It looks like you are referring to the ATS 5.3.x branch in your comment above. This looks like it has been resolved on master and 7.1.x. Please confirm.

Looks like commit 9399a76 patched this particular place (for TS-3841).

I'm not sure what you're suggesting, Bryan, is different from the ink_release_assert on line 776 here. len shouldn't just be greater than zero, it should be at least INK_MIN_ALIGN.

I am stating this is not an issue for master and 7.1.x, if you want to add an assert that is OK, but is it not required. I would keep what is in LogField.cc and get rid of the rest of the changes.

This is not the only logging anomaly we are seeing so I suspect it is in fact an issue elsewhere. I would rather do a basic fix rather than patching after the fact.

SolidWallOfCode · 2017-06-09T14:43:16Z

My view is that INVALID_STR should be removed or left const char* and m_arena->str_alloc should be used if there is no valid URL to reference. That should be sufficiently cheap to be viable as in almost all cases there will be a URL.

bryancall · 2017-06-09T15:47:46Z

proxy/logging/LogField.cc

  for (LogField *f = first(); f; f = next(f)) {
    if (f->type() != LogField::sINT) {
-      bytes += f->marshal_len(lad);
+      const int len = f->marshal_len(lad);


I am stating this is not an issue for master and 7.1.x, if you want to add an assert that is OK, but is it not required. I would keep what is in LogField.cc and get rid of the rest of the changes.

bryancall · 2017-06-09T16:12:28Z

The main issues I have with this PR:

I don't like that the invalid string has a length of 1, but the string length is being set to 0. I would rather stay with the length of -1 to mean invalid and the code is set to honor this. Maybe moving to "" and a length of 0 would work, but then all the code should change that is looking for invalid as being -1.
const variables should be used in favor of #define.

a-canary · 2017-06-09T21:21:26Z

The invalid string is not expected to be printed. It is merely a non-null address so that validation does not run multiple times. In the case that lookup url or canon host fails to resolve, the _len remains 0, thus the default "-" is printed in marshal_mem.
We are trying to move away from the strlen of -1 initialization, because there are many functions where that has undefined behaviour, or it casted to unsigned type, which is scary.

a-canary · 2017-06-09T22:19:48Z

I just pushed another commit to address the concerns above. Please review. Susan is testing this on one of the ats 7 production (test) boxes. So far so good.
If that goes well, I'll do another pull request for the 5.3 integration on Monday.

a-canary · 2017-06-12T17:46:23Z

It has survived a night in the wild. Are there any remaining concerns?

Code was using -1 string length to signify that had not been validated yet. This caused marshal_mem to allocate 0 bytes, below the INK_MIN_ALIGN, and write to unallocated mem. Now code initializes all string length vars to 0, and upon failure to validate, the string ptr is set to INVALID_STR to prevent multiple validataions attempts. Fixes YTSATS-1240 + Removed const_cast on INVALID_STR + removed len < 0 checks. + Replaced 0 >= len checks with str == INVALID_STR format

a-canary · 2017-06-13T14:58:40Z

Fixed format and squashed. It is ready to go now.

Aaron fixed his email problem and the commit data is good now.

jpeach previously requested changes Jun 8, 2017

View reviewed changes

bryancall added the Logging label Jun 8, 2017

a-canary force-pushed the LogAccessHttp_mem_overwrite_fix-master branch from dceb724 to c336517 Compare June 8, 2017 20:54

bryancall reviewed Jun 8, 2017

View reviewed changes

bryancall requested changes Jun 9, 2017

View reviewed changes

bryancall assigned a-canary Jun 12, 2017

a-canary force-pushed the LogAccessHttp_mem_overwrite_fix-master branch from 8d6fad3 to ed5a260 Compare June 13, 2017 14:57

bryancall approved these changes Jun 13, 2017

View reviewed changes

SolidWallOfCode merged commit d0e9277 into apache:master Jun 14, 2017

bryancall added this to the 8.0.0 milestone Jun 14, 2017

a-canary mentioned this pull request Jun 29, 2020

Prevent buffer overflow during log filter actions #6950

Merged

LogAccessHttp init strlen to 0, not -1 (master) #2102

LogAccessHttp init strlen to 0, not -1 (master) #2102

Uh oh!

Conversation

a-canary commented Jun 8, 2017

Uh oh!

jpeach left a comment

Choose a reason for hiding this comment

Uh oh!

zwoop commented Jun 8, 2017

Uh oh!

bryancall commented Jun 8, 2017

Uh oh!

a-canary commented Jun 8, 2017

Uh oh!

jpeach commented Jun 8, 2017 via email

Uh oh!

a-canary commented Jun 8, 2017

Uh oh!

leighklotz commented Jun 8, 2017

Uh oh!

a-canary commented Jun 8, 2017

Uh oh!

bryancall Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-canary Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

bryancall Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

bryancall Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

bryancall Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Jun 10, 2017

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Jun 9, 2017

Uh oh!

bryancall Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryancall commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-canary commented Jun 9, 2017

Uh oh!

a-canary commented Jun 9, 2017

Uh oh!

a-canary commented Jun 12, 2017

Uh oh!

a-canary commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bryancall Jun 8, 2017 •

edited

Loading

bryancall Jun 9, 2017 •

edited

Loading

bryancall Jun 9, 2017 •

edited

Loading

bryancall commented Jun 9, 2017 •

edited

Loading