Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uglify -c changes behavior of mdast code #751

Closed
tmcw opened this issue Jul 21, 2015 · 8 comments
Closed

uglify -c changes behavior of mdast code #751

tmcw opened this issue Jul 21, 2015 · 8 comments

Comments

@tmcw
Copy link
Contributor

tmcw commented Jul 21, 2015

I've created a repo to reproduce this bug: https://github.com/tmcw/mdast-uglify-bug

For the mdast markdown library, the source succeeds when not uglified, and then, passed through uglify -c, its behavior changes and it breaks.

I'm trying to dig through the source, passed through uglify -c and then uglify -b, in order to track down the cause. It's quite a doozy

@wooorm
Copy link

wooorm commented Jul 22, 2015

I fixed this in mdast (syntax-tree/mdast@1a4fc46), but I’m not sure that this was really my error.

It was basically a long list of logical AND-operators, followed by an expression which I needed the value of (rules[name].exec(value)). For some reason I did not get that value, but true.

@mishoo
Copy link
Owner

mishoo commented Jul 22, 2015

This looks like a bug. Thanks for the test repo, it helped isolating the problem. The simplest test case appears to be:

match = !x &&
    (!z || c) &&
    (!k || d) &&
    the_stuff();

compresses to:

match = !(x || z && !c || k && !d || !the_stuff());

which obviously loses the value returned from the_stuff(). Will investigate as soon as I can.

@mishoo mishoo closed this as completed in 905b601 Jul 22, 2015
@mishoo
Copy link
Owner

mishoo commented Jul 22, 2015

Fix pushed.

@tmcw
Copy link
Contributor Author

tmcw commented Jul 22, 2015

👍 awesome, thank you!

@mishoo
Copy link
Owner

mishoo commented Jul 22, 2015

I also published v2.4.24 to npm, since the issue appears to be pretty serious.

@reedloden
Copy link

reedloden commented Aug 24, 2015

Requested a CVE assignment in http://seclists.org/oss-sec/2015/q3/351

@diracdeltas
Copy link

diracdeltas commented Aug 24, 2015

I'm the author of the blog post @reedloden linked above - just wanted to clarify a couple things:

  1. I haven't found exploits in the wild for this bug.
  2. The PoC's described in the blog post don't work with the current release of UglifyJS2. (Excellent job getting things fixed quickly, @mishoo!)
  3. I suspect that it's not uncommon for minifiers, transpilers, and other js processors to accidentally change return values, which of course can lead to security problems.

reedloden added a commit to reedloden/jquery that referenced this issue Aug 24, 2015
Update grunt-contrib-uglify dependency to v0.9.2 in order to
fix security issue fixed in uglify-js v2.4.24.

mishoo/UglifyJS#751
https://zyan.scripts.mit.edu/blog/backdooring-js/
reedloden added a commit to reedloden/jquery that referenced this issue Aug 24, 2015
Update grunt-contrib-uglify dependency to v0.9.2 in order to
fix security issue fixed in uglify-js v2.4.24.

mishoo/UglifyJS#751
https://zyan.scripts.mit.edu/blog/backdooring-js/
@rvanvelzen
Copy link
Collaborator

rvanvelzen commented Aug 24, 2015

I suspect that it's not uncommon

Definitely. Most of the stuff that can bite you is behind --unsafe, but there's probably more to be found.

gracewashere added a commit to thoughtbot/administrate that referenced this issue Aug 27, 2015
Based on the recommendations of [bundler-audit]:

```
ruby-advisory-db: 227 advisories
Name: uglifier
Version: 2.7.0
Advisory: OSVDB-126747
Criticality: Unknown
URL: mishoo/UglifyJS#751
Title: uglifier incorrectly handles non-boolean comparisons during
minification
Solution: upgrade to >= 2.7.2

Unpatched versions found!
```

[bundler-audit]: https://rubygems.org/gems/bundler-audit
notalex added a commit to blackducksoftware/ohloh-ui that referenced this issue Aug 28, 2015
elliotcm added a commit to alphagov/trade-tariff-frontend that referenced this issue Sep 2, 2015
Resolves advisory 126747
mishoo/UglifyJS#751
mgol pushed a commit to jquery/jquery that referenced this issue Sep 7, 2015
Update grunt-contrib-uglify dependency to v0.9.2 in order to
avoid a security issue fixed in uglify-js v2.4.24.

mishoo/UglifyJS#751
https://zyan.scripts.mit.edu/blog/backdooring-js/

Closes gh-2556
mgol pushed a commit to jquery/jquery that referenced this issue Sep 7, 2015
Update grunt-contrib-uglify dependency to v0.9.2 in order to
avoid a security issue fixed in uglify-js v2.4.24.

mishoo/UglifyJS#751
https://zyan.scripts.mit.edu/blog/backdooring-js/

(cherry-picked from 835e921)

Closes gh-2556
akestner added a commit to projecthire/pyrite that referenced this issue Sep 8, 2015
```
Name: uglifier
Version: 2.7.1
Advisory: 126747
Criticality: Unknown
URL: mishoo/UglifyJS#751
Title: uglifier incorrectly handles non-boolean comparisons during minification
Solution: upgrade to >= 2.7.2
```
gracewashere added a commit to sfbrigade/sf-openreferral that referenced this issue Sep 10, 2015
Problem:

[bundler-audit] returned this security warning:

```
Updating ruby-advisory-db ...
From https://github.com/rubysec/ruby-advisory-db
 * branch            master     -> FETCH_HEAD
 Already up-to-date.
 ruby-advisory-db: 230 advisories
 Name: uglifier
 Version: 2.7.1
 Advisory: OSVDB-126747
 Criticality: Unknown
 URL: mishoo/UglifyJS#751
 Title: uglifier incorrectly handles non-boolean comparisons during
 minification
 Solution: upgrade to >= 2.7.2

 Unpatched versions found!
```

[bundler-audit]: https://github.com/rubysec/bundler-audit

Solution: Upgrade `bundle update uglifier`
dylangrafmyre added a commit to shakacode/react_on_rails that referenced this issue Sep 15, 2015
Ruby405 added a commit to Ruby405/Build-administrate-ruby that referenced this issue Mar 21, 2022
Based on the recommendations of [bundler-audit]:

```
ruby-advisory-db: 227 advisories
Name: uglifier
Version: 2.7.0
Advisory: OSVDB-126747
Criticality: Unknown
URL: mishoo/UglifyJS#751
Title: uglifier incorrectly handles non-boolean comparisons during
minification
Solution: upgrade to >= 2.7.2

Unpatched versions found!
```

[bundler-audit]: https://rubygems.org/gems/bundler-audit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants