New issue
Advanced search Search tips

Issue 774382 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: Persian Calendar Integer overflow lead to OOB read

Reported by scdengy...@gmail.com, Oct 13 2017

Issue description

VULNERABILITY DETAILS
PersianCalendar::handleComputeFields in persncal.cpp did not properly process julianDay, lead to interger overflow:
too big julianDay ==> too big month ==> kPersianNumDays[month] out of bound read

VERSION
Version Version 61.0.3163.100 (Official Build) (64-bit)
Operating System: [Mac OS, 10.12.6]


 
exp.js
1.0 KB View Download
crash.log
5.4 KB View Download
Project Member

Comment 1 by ClusterFuzz, Oct 13 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4976653825736704.
Components: Blink>JavaScript>Internationalization
Project Member

Comment 3 by ClusterFuzz, Oct 13 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6110814028431360.
For some reason, I can't get this to blow up on Clusterfuzz, but the repro does behave differently in each Chrome version and does not match Firefox. The values returned from formatToParts have values that are plainly illegal:

[{"type":"era","value":"AP"},{"type":"literal","value":" "},{"type":"year","value":"0"},{"type":"literal","value":"-"},{"type":"month","value":"-4198391"},{"type":"literal","value":"-"},{"type":"day","value":"-130167510"}];
It appears that the Hebrew calendar implementation had an issue like this in the past:

https://cs.chromium.org/chromium/src/third_party/icu/source/i18n/hebrwcal.cpp?l=591&rcl=21d33b1a09a77f033478ea4ffffb61e6970f83bd
    if (month >= momax || month<=0) {
        // TODO: I found dayOfYear could be out of range when
        // a large value is set to julianDay.  I patched startOfYear
        // to reduce the chace, but it could be still reproduced either
        // by startOfYear or other places.  For now, we check
        // the month is in valid range to avoid out of array index
        // access problem here.  However, we need to carefully review
        // the calendar implementation to check the extreme limit of
        // each calendar field and the code works well for any values
        // in the valid value range.  -yoshito
        status = U_ILLEGAL_ARGUMENT_ERROR;
        return;
    }
Labels: Pri-2
Owner: js...@chromium.org
Status: Assigned (was: Unconfirmed)

Comment 7 by kenrb@chromium.org, Oct 17 2017

Labels: Security_Severity-Medium Security_Impact-Stable OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 8 by tsepez@chromium.org, Oct 17 2017

Labels: M-63
Project Member

Comment 9 by sheriffbot@chromium.org, Oct 18 2017

Labels: -Pri-2 Pri-1
I think the Security_Severity should be "High"? In my exp.js, when given proper 'i' in the for loop, we can leak the hole memory. In the array kPersianNumDays[month], the index 'month' can be both overflowed & underflowed. Leverage that, we can bypass ASLR or leak some critical information.
It's certainly the case that this bug could be used as a component in a longer chain of vulnerabilities, but typically, OOB reads in a Renderer process are triaged at Severity Medium: https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md 
ok, sorry I didn't know that guidelines.
Project Member

Comment 13 by sheriffbot@chromium.org, Oct 27 2017

jshin: 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
ICU 60 has a change to deal with an integer overflow, but it seems that it's a different part than this bug report refers to. 

I couldn't verify that the change in ICU 60 also fixes this because I can't reproduce a crash with Asan build. 

I'll see if formatToParts looks saner with ICU 60 than with ICU 59. 
scdengyuan@ : can you try the latest ToT v8 (which uses ICU 60) to see if you can still reproduce this issue?  

I think this line fixed this bugs:
-    year = 1 + ClockMath::floorDivide(33 * daysSinceEpoch + 3, 12053);
+    year = 1 + (int32_t)ClockMath::floorDivide(33 * (int64_t)daysSinceEpoch + 3, (int64_t)12053);
I just tried the latest v8, but can not reproduce this bug.

Comment 17 by js...@chromium.org, Nov 13 2017

Thank you,  scdengyuan@ !

-------------
-    year = 1 + ClockMath::floorDivide(33 * daysSinceEpoch + 3, 12053);
+    year = 1 + (int32_t)ClockMath::floorDivide(33 * (int64_t)daysSinceEpoch + 3, (int64_t)12053);

---------

The above is the change I talked about in comment 14, but I couldn't verify it indeed fix the issue reported here because I couldn't reproduce it even without that change. 

Because scdengyuan@ was able to verify that the above change indeed fixed this issue, I'll port that change back to 63 branch. 



Project Member

Comment 18 by sheriffbot@chromium.org, Nov 28 2017

jshin: 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: Started (was: Assigned)
I started porting ICU 60's change to ICU 59.1 used by Chrome 63.x. 
Chrome 64.x is fine. 
Summary: Security: Persian Calendar Integer overflow lead to OOB read (was: Security: PersianCalendar Integer overflow lead to OOB read)
Labels: Merge-Request-63
Requesting for merge of the following change in ICU to M63 branch. 

https://chromium.googlesource.com/chromium/deps/icu/+/29a98cea94c967fdd33ff51a0920ebc9e7c3cfba

The change is in M64 as a part of ICU update to 60.1 and it has been baked in ToT for about 3 weeks. This merge request is to cherry-pick a change in ICU 60.1 back to ICU 59.1 used by Chrome M63 branch. 

It's a 1-line change (comment 16) and should be safe. 


Project Member

Comment 22 by sheriffbot@chromium.org, Dec 4 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: We are only 0 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
ICU in M63 will be deps-rolled to 29a98cea94c96. 
Cc: awhalley@chromium.org
+awhalley@ for M63 merge review 
Project Member

Comment 25 by sheriffbot@chromium.org, Dec 4 2017

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This has missed the first M63 stable cut, so we'll wait for this to get into dev/beta first before merging to stable.
 awhalley@ : yeah, I missed M63 stable train. (I should have done cherry-pick earlier :-) ). 

Just in case it was not clear,  this change has been in 64.x branches for 3+ weeks and was included in two dev channel releases; 64.0.3269.3 and 64.0.3278.0 ) because ICU was updated to ICU 60.1 in early November. ICU 60.1 does have this change. 

The merge request is to deps-roll ICU for Chrome M63 branch to a version of ICU 59.1 with a one-line cherry-pick of the fix from ICU 60.1. 




thanks for the details, jshin@ - I did miss the date of the first commit.

govind@ - good for 63
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comment #28. Please merge ASAP as we're cutting M63 stable RC soon. Thank you.
Project Member

Comment 30 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/96e7d2237ee06551f646fdb8fdf9e96ccf124daa

commit 96e7d2237ee06551f646fdb8fdf9e96ccf124daa
Author: Jungshik Shin <jungshik@google.com>
Date: Mon Dec 04 18:44:57 2017

Labels: reward-topanel
Labels: -Merge-Approved-63 merge-merged-3239
Labels: Release-0-M63
Project Member

Comment 34 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/77128f4279529fe9203d6093d72f17f36f2d8998

commit 77128f4279529fe9203d6093d72f17f36f2d8998
Author: Jungshik Shin <jungshik@google.com>
Date: Mon Dec 04 20:25:42 2017

Labels: CVE-2017-15422
Project Member

Comment 36 by sheriffbot@chromium.org, Dec 5 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -reward-topanel reward-0
Hi scdengyuan@ - I'm afraid the VRP panel declined to reward for this report, since it looks like the bug in question was already fixed upstream. Still, many thanks!
Project Member

Comment 38 by sheriffbot@chromium.org, Mar 12 2018

Labels: -Restrict-View-SecurityNotify allpublic
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
Project Member

Comment 39 by sheriffbot@chromium.org, Mar 27 2018

Labels: -M-63 M-65
Labels: CVE_description-missing
Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment