Closed Bug 1374012 Opened 3 years ago Closed 2 years ago

Update to Expat 2.2.1

Categories

(Core :: XML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 65+ fixed
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: tsmith, Assigned: peterv)

References

Details

(Keywords: sec-high, Whiteboard: [third-party-lib-audit][post-critsmash-triage][adv-main65-][adv-esr60.5-])

Attachments

(19 files, 18 obsolete files)

47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
47 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
Update expat files that live in: parser/expat/lib/

For list of fixed CVEs see:
http://www.openwall.com/lists/oss-security/2017/06/17/7
This fixes some integer overflows, a double free and more. So marking s-s for now.
Group: dom-core-security
Priority: -- → P2
FWIW I've explicitly avoided updating to the latest expat versions as they've tend to introduce more CVE's than they fix. We keep a much trimmed down (and modified) version of 2.0.0 in tree, it would be interesting to see what overlap there is and maybe just cherry-pick changes that are relevant to us.
I've started looking over the differences. I'll attach some patches with some no-brainers and then we can decide on the rest.
Assignee: nobody → peterv
From the release notes:

   CVE-2017-9233  External entity infinite loop DoS

Probably affects us, I don't recall triaging anything similar.

  (CVE-2016-9063) Integer overflow (re-fix)

This is bug 1274777.

             n/a  More integer overflow fixes

Needs auditing.

  (CVE-2016-0718) Fix regression bugs from 2.2.0's fix to CVE-2016-0718

This bug 1236923.

  (CVE-2016-5300) Use os-specific entropy sources like getrandom
  (CVE-2012-0876) Counter hash flooding with SipHash

I think these are kind of the same, I believe they're dealing with hash collisions causing DoS.

First commit to seed their basic hasher with srand:
   https://github.com/libexpat/libexpat/commit/e3e81a6d9f0885ea02d3979151c358f314bf3d6
Example of follow up to use better random sources (there are a few like this):
   https://github.com/libexpat/libexpat/commit/01e78c377b5d348d99b31e22129f2053d0b7596d
Follow up to use SipHash:
   https://github.com/libexpat/libexpat/commit/3fcef5021abf2de7cb61c8716a4674017114b

             n/a  No longer leak parser pointer information
             n/a  Prevent use of uninitialised variables
             n/a  Add missing API parameter validation (NULL, len<0)

Need auditing.
Keywords: sec-high
(In reply to Peter Van der Beken [:peterv] from comment #3)
> I've started looking over the differences. I'll attach some patches with
> some no-brainers and then we can decide on the rest.

Do you have a timeline?
Flags: needinfo?(peterv)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #4)
> From the release notes:
> 
>    CVE-2017-9233  External entity infinite loop DoS
> 
> Probably affects us, I don't recall triaging anything similar.

We can take this, but I don't think this is a threat for us. We don't load external DTDs except from chrome/res URIs, so we have control over what DTDs get loaded.

>   (CVE-2016-9063) Integer overflow (re-fix)
> 
> This is bug 1274777.

We can just merge it in, but this is already fixed.

>              n/a  More integer overflow fixes
> 
> Needs auditing.

Probably should take this.

>   (CVE-2016-0718) Fix regression bugs from 2.2.0's fix to CVE-2016-0718
> 
> This bug 1236923.

We can just merge it in, but this is already fixed.

>   (CVE-2016-5300) Use os-specific entropy sources like getrandom
>   (CVE-2012-0876) Counter hash flooding with SipHash
> 
> I think these are kind of the same, I believe they're dealing with hash
> collisions causing DoS.
> 
> First commit to seed their basic hasher with srand:
>   
> https://github.com/libexpat/libexpat/commit/
> e3e81a6d9f0885ea02d3979151c358f314bf3d6
> Example of follow up to use better random sources (there are a few like
> this):
>   
> https://github.com/libexpat/libexpat/commit/
> 01e78c377b5d348d99b31e22129f2053d0b7596d
> Follow up to use SipHash:
>   
> https://github.com/libexpat/libexpat/commit/
> 3fcef5021abf2de7cb61c8716a4674017114b

We should take this, but it looks non-trivial.

>              n/a  No longer leak parser pointer information
>              n/a  Prevent use of uninitialised variables
>              n/a  Add missing API parameter validation (NULL, len<0)
> 
> Need auditing.

A lot of these don't affect us. They're validating arguments for values that we never pass in. Still going through these though.

So I think so far the important ones are the overflow fixes and probably the hashing.
Flags: needinfo?(peterv)
Attached patch Part 1: overflow checks (obsolete) — Splinter Review
Hey Peter, just for clarification, who do you want to sync up with to make the decision which patches we take? Someone from security or someone working on XML? The items in comment 6 did not really address anyone in particular.
Flags: needinfo?(peterv)
Joe: if this isn't peter's area anymore can you help find a new owner for this security bug?
Flags: needinfo?(jwalker)
Meanwhile a few more expat updates have been released, but the security fixes in 2.2.2 and 2.2.3 don't look like it's functionality we'd use (poor entropy gathering in some conditions, windows DLL hijacking)
https://github.com/libexpat/libexpat/blob/R_2_2_4/expat/Changes
(In reply to Daniel Veditz [:dveditz] from comment #9)
> Joe: if this isn't peter's area anymore can you help find a new owner for
> this security bug?

I can look at this if Peter doesn't have time.
Had a conversation with peterv and agreed that he's going to work with erahm to land his set of patches to upgrade expat.
Flags: needinfo?(jwalker)
Attached patch Part 2: Better hashing (obsolete) — Splinter Review
Attached patch Part 3: Reject invalid DTD (obsolete) — Splinter Review
Flags: needinfo?(peterv)
Comment on attachment 8926066 [details] [diff] [review]
Part 2: Better hashing

Review of attachment 8926066 [details] [diff] [review]:
-----------------------------------------------------------------

So I was hoping we could use our sip hasher [1], but that's siphash 1-3 and this is siphash 2-4 so maybe it's unavoidable. It's possible we could just use HashString combined with HashScrambler. It would also be nice to use NSS's random byte support (ie [2]) rather than whatever they came up with as good enough.

In general I'd much prefer if expat provided hooks for hashing algorithms and sources of random bytes so we could provide gecko-based solutions, but this is probably all good enough so maybe it's not a big deal.

[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/mfbt/HashFunctions.h#297-312
[2] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/base/Crypto.cpp#94-102
Comment on attachment 8926066 [details] [diff] [review]
Part 2: Better hashing

Review of attachment 8926066 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/expat/lib/xmlparse.c
@@ +806,5 @@
> +# include <bsd/stdlib.h>
> +#endif
> +
> +static unsigned long
> +ENTROPY_DEBUG(const char * label, unsigned long entropy) {

This definitely should go, we should not be shipping debug code that's hitting getenv and fprintf'ing.
Are you still able to work on landing these patches Peter?
Flags: needinfo?(peterv)
Another CVE was patched in Expat 2.2.3 on August 2 2017:

http://www.openwall.com/lists/oss-security/2017/08/02/4
I ran a script through the commits since 2.0.0.

It found a few things that are not mentioned explicitly above, but spot checking the patch shows that at least some of them are included.

CVE-2012-1148
CVE-2012-1147
CVE-2009-3560
CVE-2009-3720
https://marcograss.github.io/security/android/chromium/2016/06/17/expat-xml-heap-overflow.html
44178553f3539ce69d34abee77a05e879a7982ac
4be2cb5afcc018d996f34bbbce6374b7befad47f
810b74e4703dcfdd8f404e3cb177d44684775143
bb1fd81b98870bd2c7628b4b011a61a9b9d791aa
4be2cb5afcc018d996f34bbbce6374b7befad47f
CVE-2015-1283
2cac066cf6ef47c250f83861b61b624cd14c293f

And then also these which were mentioned above:
CVE-2012-0876
CVE-2017-9233
CVE-2016-9063
CVE-2015-1283 - this was us :)


Expat does not have a very clean commit pattern, so some of those are likely false positives; but they definitely show a number of open issues in our version of expat.
We also need to take into consideration that a fair amount of the fixes are for bugs introduced post 2.0.0 or in parts of the code we don't use (UTF8 conversion, etc).
(In reply to Tom Ritter [:tjr] from comment #34)
> I ran a script through the commits since 2.0.0.
> 
> It found a few things that are not mentioned explicitly above, but spot
> checking the patch shows that at least some of them are included.

Just to be clear which ones are fixed:

> CVE-2012-1148

Peter addresses this in patch 1.

> CVE-2012-1147

This is in readfilemap.c, we don't ship that.

> CVE-2009-3560

This is in character conversion which we don't use.

> CVE-2009-3720

This is character conversion which we don't use.

> https://marcograss.github.io/security/android/chromium/2016/06/17/expat-xml-
> heap-overflow.html

I can't find the exact bug, but I'm pretty sure I looked into this and it's not an issue for Fx, we could have someone run that code under asan of course to double-check.

> 44178553f3539ce69d34abee77a05e879a7982ac

Peter addresses this in patch 1.

> 4be2cb5afcc018d996f34bbbce6374b7befad47f

Peter addresses this in patch 1.

> 810b74e4703dcfdd8f404e3cb177d44684775143

Peter addresses this in patch 1.

> bb1fd81b98870bd2c7628b4b011a61a9b9d791aa

This looks like encoding stuff we don't use.

> 4be2cb5afcc018d996f34bbbce6374b7befad47f

This is a dup, see above.

> CVE-2015-1283

As noted below, this is https://www.mozilla.org/en-US/security/advisories/mfsa2015-54/.

> 2cac066cf6ef47c250f83861b61b624cd14c293f

Peter addresses this in patch 1.

> 
> And then also these which were mentioned above:
> CVE-2012-0876

This is part 2.

> CVE-2017-9233

This is part 3.

> CVE-2016-9063

This is also us, bug 1274777.

> CVE-2015-1283 - this was us :)
> 
> 
> Expat does not have a very clean commit pattern, so some of those are likely
> false positives; but they definitely show a number of open issues in our
> version of expat.

Yes their tracking scheme is unfortunate.
The patches as-is have some build failures. Trying to address those now.
Flags: needinfo?(peterv)
(In reply to Peter Van der Beken [:peterv] from comment #37)
> The patches as-is have some build failures. Trying to address those now.

It's been a while :) Any news?
Flags: needinfo?(peterv)
From conversations with Eric and Peter, it doesn't appear we have a great path forward here.
Flags: needinfo?(peterv)
(In reply to Andrew Overholt [:overholt] from comment #39)
> From conversations with Eric and Peter, it doesn't appear we have a great
> path forward here.

Well we could work out the build failures, taking this patchset seems reasonable vs a wholesale upgrade.
Attachment #9031154 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 1: More correct calculations. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 1: More correct calculations. r=erahm
Attachment #9031156 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 2b: cherry-pick compilation fix from 2.2.2. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 2b: cherry-pick compilation fix from 2.2.2. r=erahm
Attachment #9031158 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 3: Reject invalid DTD. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 3: Reject invalid DTD. r=erahm
Attachment #9031160 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 4: removing mainlined customisations and merge whitespace changes. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 4: removing mainlined customisations and merge whitespace changes. r=erahm
Attachment #9031162 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 5: Use ASCII_* instead of literal characters. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 5: Use ASCII_* instead of literal characters. r=erahm
Attachment #9031163 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 6: Add XML_ATTR_INFO code which we don't enable. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 6: Add XML_ATTR_INFO code which we don't enable. r=erahm
Attachment #9031165 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 7: Add __unused__ anotations. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 7: Add __unused__ anotations. r=erahm
Attachment #9031164 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 8: Validate parser argument in various APIs. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 8: Validate parser argument in various APIs. r=erahm
Attachment #9031166 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 9: Make XmlConvert return errors. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 9: Make XmlConvert return errors. r=erahm
Attachment #9031167 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 10: Take into account that CHAR_MATCHES may read >1 bytes. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 10: Take into account that CHAR_MATCHES may read >1 bytes. r=erahm
Attachment #9031168 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 11: Changes to defines for various compilers. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 11: Changes to defines for various compilers. r=erahm
Attachment #9031171 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 12: Address warning "missing initializer for field". r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 12: Address warning "missing initializer for field". r=erahm
Attachment #9031172 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 13: Tidy up attribute prefix bindings on error. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 13: Tidy up attribute prefix bindings on error. r=erahm
Attachment #9031173 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 14: Annotate memory allocators for GCC. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 14: Annotate memory allocators for GCC. r=erahm
Attachment #9031174 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 15: ifdef some constants so they are only defined when used. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 15: ifdef some constants so they are only defined when used. r=erahm
Attachment #9031176 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 16: fix some issues with various compilers. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 16: fix some issues with various compilers. r=erahm
Attachment #9031177 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 17: add/change APIs that are not used in Gecko. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 17: add/change APIs that are not used in Gecko. r=erahm
Attachment #9031178 - Attachment description: Bug 1374012 - Update to Expat 2.2.1. Part 18: various miscellaneous changes. r?erahm! → Bug 1374012 - Update to Expat 2.2.1. Part 18: various miscellaneous changes. r=erahm
Attachment #8887094 - Attachment is obsolete: true
Attachment #8926066 - Attachment is obsolete: true
Attachment #8926067 - Attachment is obsolete: true
Attachment #8926068 - Attachment is obsolete: true
Attachment #8926069 - Attachment is obsolete: true
Attachment #8926070 - Attachment is obsolete: true
Attachment #8926071 - Attachment is obsolete: true
Attachment #8926072 - Attachment is obsolete: true
Attachment #8926075 - Attachment is obsolete: true
Attachment #8926077 - Attachment is obsolete: true
Attachment #8926079 - Attachment is obsolete: true
Attachment #8926080 - Attachment is obsolete: true
Attachment #8926081 - Attachment is obsolete: true
Attachment #8926082 - Attachment is obsolete: true
Attachment #8926083 - Attachment is obsolete: true
Attachment #8926084 - Attachment is obsolete: true
Attachment #8926086 - Attachment is obsolete: true
Attachment #8926087 - Attachment is obsolete: true
Comment on attachment 9031154 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 1: More correct calculations. r=erahm

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Probably not hard, the original bug reports for upstream might contain examples.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown

Which older supported branches are affected by this flaw?: All

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Should be trivial.

How likely is this patch to cause regressions; how much testing does it need?: Medium risk I think, it's a large number of changes in the XML parser. On the other hand, these have been shipping for a while in other projects.
Attachment #9031154 - Flags: sec-approval?
Comment on attachment 9031155 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 2a: Better hashing. r?erahm!

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: See comment 65

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031155 - Flags: sec-approval?
Comment on attachment 9031156 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 2b: cherry-pick compilation fix from 2.2.2. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031156 - Flags: sec-approval?
Comment on attachment 9031158 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 3: Reject invalid DTD. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031158 - Flags: sec-approval?
Comment on attachment 9031160 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 4: removing mainlined customisations and merge whitespace changes. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031160 - Flags: sec-approval?
Comment on attachment 9031162 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 5: Use ASCII_* instead of literal characters. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031162 - Flags: sec-approval?
Comment on attachment 9031163 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 6: Add XML_ATTR_INFO code which we don't enable. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031163 - Flags: sec-approval?
Comment on attachment 9031164 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 8: Validate parser argument in various APIs. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031164 - Flags: sec-approval?
Comment on attachment 9031165 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 7: Add __unused__ anotations. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031165 - Flags: sec-approval?
Comment on attachment 9031166 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 9: Make XmlConvert return errors. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031166 - Flags: sec-approval?
Comment on attachment 9031167 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 10: Take into account that CHAR_MATCHES may read >1 bytes. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031167 - Flags: sec-approval?
Comment on attachment 9031168 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 11: Changes to defines for various compilers. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031168 - Flags: sec-approval?
Comment on attachment 9031171 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 12: Address warning "missing initializer for field". r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031171 - Flags: sec-approval?
Comment on attachment 9031172 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 13: Tidy up attribute prefix bindings on error. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031172 - Flags: sec-approval?
Comment on attachment 9031173 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 14: Annotate memory allocators for GCC. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031173 - Flags: sec-approval?
Comment on attachment 9031174 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 15: ifdef some constants so they are only defined when used. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031174 - Flags: sec-approval?
Comment on attachment 9031176 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 16: fix some issues with various compilers. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031176 - Flags: sec-approval?
Comment on attachment 9031177 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 17: add/change APIs that are not used in Gecko. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031177 - Flags: sec-approval?
Comment on attachment 9031178 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 18: various miscellaneous changes. r=erahm

See comment 65

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9031178 - Flags: sec-approval?
Comment on attachment 9031178 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 18: various miscellaneous changes. r=erahm

sec-approval+ for trunk. I expect we should backport this...
Attachment #9031178 - Flags: sec-approval? → sec-approval+
Attachment #9031177 - Flags: sec-approval? → sec-approval+
Attachment #9031154 - Flags: sec-approval? → sec-approval+
Attachment #9031155 - Flags: sec-approval? → sec-approval+
Attachment #9031156 - Flags: sec-approval? → sec-approval+
Attachment #9031158 - Flags: sec-approval? → sec-approval+
Attachment #9031160 - Flags: sec-approval? → sec-approval+
Attachment #9031162 - Flags: sec-approval? → sec-approval+
Attachment #9031163 - Flags: sec-approval? → sec-approval+
Attachment #9031164 - Flags: sec-approval? → sec-approval+
Attachment #9031165 - Flags: sec-approval? → sec-approval+
Attachment #9031166 - Flags: sec-approval? → sec-approval+
Attachment #9031167 - Flags: sec-approval? → sec-approval+
Attachment #9031168 - Flags: sec-approval? → sec-approval+
Attachment #9031171 - Flags: sec-approval? → sec-approval+
Attachment #9031172 - Flags: sec-approval? → sec-approval+
Attachment #9031173 - Flags: sec-approval? → sec-approval+
Attachment #9031174 - Flags: sec-approval? → sec-approval+
Attachment #9031176 - Flags: sec-approval? → sec-approval+
These patches all graft cleanly to Beta and ESR60 as-landed. Please request approval when you get a chance. For both of our sanity's sake, you can do the requests on just Part 1 with a note that it applies to all 19 :).
Flags: needinfo?(peterv)
Comment on attachment 9031154 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 1: More correct calculations. r=erahm

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Potential security bugs.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed: 

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: Potential security bugs.

Fix Landed on Version: 66

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Large number of changes in the XML parser. On the other hand, these have been shipping for a while in other projects.

String or UUID changes made by this patch:
Flags: needinfo?(peterv)
Attachment #9031154 - Flags: approval-mozilla-esr60?
Attachment #9031154 - Flags: approval-mozilla-beta?
As noted in comment 87, the beta/ESR approval request should be considered to apply to parts 1 to 19.
Comment on attachment 9031154 [details]
Bug 1374012 - Update to Expat 2.2.1. Part 1: More correct calculations. r=erahm

[Triage Comment]
Fixes XML parser security issues. Approved for 65.0b8 and 60.5.0esr.
Attachment #9031154 - Flags: approval-mozilla-esr60?
Attachment #9031154 - Flags: approval-mozilla-esr60+
Attachment #9031154 - Flags: approval-mozilla-beta?
Attachment #9031154 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [third-party-lib-audit] → [third-party-lib-audit][post-critsmash-triage]
Whiteboard: [third-party-lib-audit][post-critsmash-triage] → [third-party-lib-audit][post-critsmash-triage][adv-main65-][adv-esr60.5-]
Depends on: 1516642
Group: core-security-release
Duplicate of this bug: 1419280
You need to log in before you can comment on or make changes to this bug.