Hello, do you have a reproduction case? Hard to tell what the bug is from the stack trace.

rmcilroy@: Adding you since bug 642111 is the only other bug that mentions SourcePositionTable, please reassign as necessary.

Adding Vogelheim and Yang who know the source position table better than me.

Am mostly confused.

- The code in AddAndSetEntry looks harmless, so I suspect the data in the table was incorrect.
- asan reports a SIGFPE, and apparently ASAN can report signed integer overflows as FPE, and the designated line in source-position-table.cc:37 adds signed integers.
- If this addition overflows, that's indeed bad.
- Just from staring at the code, I can't find any reason why it would, though.

So without further data...  I guess I could turn the comparison (index_ == table_->length()) in SourcePositionTableIterator::Advance() into a >=. That's guessing, though, and I'd very much prefer to actually understand the problem. (Quite likely it's in how we build up the table in the first place, not when/how we read it.)


@revskills: Could you share the input with us? I notice you're running this from v8/samples/shell.cc, so presumably you already have the input as stand-along .js files.

I'm far away from my computer untill next week. So Unfortunately not. Browsing the source code It's what I thought. Fix seems valid to me in that case. Probably will be a good idea to apply it, I will mention it (just stacktrace) in a talk about fuzzing js engines.

++ this week.

A "fix" by adding a >= comparison is on its way, as discussed in #3/#4.

That fix is conservative (in that I'm sure it won't break anything) & rather speculative (in that I'm largely guessing, as I don't have enough evidence to determine the problem exactly.) I strongly suspect I'm merely fixing the symptom and that the actual bug is sometime earlier; maybe when we build up the table.


@revskills: Would be nice if you could submit a full repro case sometime later; there's certainly no hurry. Being able to reproduce this would help me fix it properly.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/da72a3176fba32b39049d070fbd2d789bbbfe8b6

commit da72a3176fba32b39049d070fbd2d789bbbfe8b6
Author: vogelheim <vogelheim@chromium.org>
Date: Wed Nov 23 13:48:31 2016

Prevent read-after-buffer in SourcePositionTableIterator::Advance.

R=yangguo@chromium.org
BUG= chromium:667142 

Review-Url: https://codereview.chromium.org/2525663003
Cr-Commit-Position: refs/heads/master@{#41224}

[modify] https://crrev.com/da72a3176fba32b39049d070fbd2d789bbbfe8b6/src/source-position-table.cc

Thank you. Any CVE allocated? Just in case I can note 

#8: I wouldn't know; I'm just a lowly compiler writer.

meacer@, would you know?

Any update?, CVE,security impact and so on?. Thank you

FPE crashes are non-security bugs by default, unless there is a reason to suspect that this could cause something other than a crash. Could you elaborate on this?

Also would you mind sharing a testcase for reproducing the issue, as requested per comments #1, #3 and #6?


According to the guidelines (https://www.chromium.org/developers/severity-guidelines) it doesn't seem be high severity anyway, so I'm setting Medium for now. Please note that it is a subject to change once we get a reproducer and/or clarifications on possible exploitation.

I'm still out from my computer, so not so far. But based on stacktrace seems obvious there are security implications, however I'm not familiar with this code and hence not determined possible explotation ways. It is because I asked to figure out what's going on with help of a mantainer. Thank you

55 is released, not mentioned this issue even with commit https://chromium.googlesource.com/v8/v8/+/da72a3176fba32b39049d070fbd2d789bbbfe8b6

Not surprising, given that this issue is not even confirmed to be one yet.

 Ok, feel free to close it.

Still very interested in your repro though!

vogelheim: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Status as of today:

- Fix went into master branch on Nov 23rd.
- I did not back merge.
- As Nov 23rd was just a few days *after* the M56 branch cut, it will ship in M57 first.
- The information is still rather incomplete. From what I'm seeing I would guess security impact to be low; but I'm not particularly qualified to assess that.
- Since the fix is based on guesswork, it's quite conceivable the real bug is still out there, but I don't really have any leads.

Given the above, I'll close as 'fixed'.


@awhalley: Could you take a quick look on whether I mis-judged? If so, please re-open.

Your change meets the bar and is auto-approved for M56 (branch: 2924)

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

To be fair this is a Medium security vulnerability. I am not interested in any reward, so you can remove 'reward-topanel' label.  

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/31b99bba343bd71c24e1e70440800488ff57b679

commit 31b99bba343bd71c24e1e70440800488ff57b679
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Mon Jan 02 12:15:57 2017

Merged: Prevent read-after-buffer in SourcePositionTableIterator::Advance.

Revision: da72a3176fba32b39049d070fbd2d789bbbfe8b6

BUG= chromium:667142 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=marja@chromium.org, hablich@chromium.org

Review-Url: https://codereview.chromium.org/2608123002 .
Cr-Commit-Position: refs/branch-heads/5.6@{#62}
Cr-Branched-From: bdd3886218dfe76e8560eb8a18401942452ae859-refs/heads/5.6.326@{#1}
Cr-Branched-From: 879f6599eee6e1dfcbe9a24bf688b261c03e9558-refs/heads/master@{#41014}

[modify] https://crrev.com/31b99bba343bd71c24e1e70440800488ff57b679/src/source-position-table.cc

I'm afraid the panel declined to reward for this, but would reconsider if you're able to provide a reproduction that shows how the crash could be security relevant.  Thanks for the report!

as per comment #27 "I am not interested in any reward, so you can remove 'reward-topanel' label." 

Cheers

Ah, sorry, I didn't spot that!  For future reference, if you decline a reward we'll happily double it and donate it to a charity of your choosing :-)

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot