openedx / wg-frontend

Open edX Frontend Working Group
4 stars 0 forks source link

frontend-build: High security vulnerability needs semver-regex update (via image-webpack-loader) #107

Closed dianekaplan closed 1 year ago

dianekaplan commented 2 years ago

High: Regular Expression Denial of Service (ReDOS)
Patched in semver-regex version >=3.1.3
dependency chain: @edx/frontend-build > image-webpack-loader > imagemin-gifsicle > gifsicle > bin-wrapper > bin-version-check > bin-version > find-versions > semver-regex
more info: https://github.com/advisories/GHSA-44c6-4v22-4mhx

It looks like frontend-build currently uses image-webpack-loader 8.1.0, which only uses semver-regex version 2.0.0. (We need to update image-webpack-loader to a version that uses semver-regex >=3.1.3).

PRs

mamankhan99 commented 2 years ago

Hi @dianekaplan, 8.1.0 is the latest version of image-webpack-loader, we can not update it at the moment. Should we wait for the patch? I would appreciate your suggestions.

jmbowman commented 2 years ago

It looks like the issue has been flagged upstream but there has not yet been a response: https://github.com/tcoopman/image-webpack-loader/issues/414 . Could you go ahead and create a PR upstream to address it?

jmbowman commented 2 years ago

@mamankhan99 Are you (or anybody else on FED-BOM) up for creating an upstream PR as suggested above?

mamankhan99 commented 2 years ago

We can but we will need to create PRs from bin-version to image-webpack-loader to fix the issue. Should we proceed?

jmbowman commented 2 years ago

Sorry for not seeing this earlier. I'm ok with that, but is there an alternative we can use instead of image-webpack-loader? The webpack docs mention image-minimizer-webpack-plugin , is that the preferred library for this functionality now? Is it in a better state of maintenance? There's some relevant discussion in https://github.com/webpack-contrib/image-minimizer-webpack-plugin/issues/225 .

mamankhan99 commented 2 years ago

Yes! we can replacte it with image-minimizer-webpack-plugin. Let me discuss the feasibility with my team and then we can start working on it.

BilalQamar95 commented 2 years ago

We are using v8.1.0 of image-webpack-loader which is the latest version. The package is bundled with following optimizers imagemin-gifsicle: Imagemin plugin for Gifsicle (Compress GIF images) imagemin-mozjpeg: Imagemin plugin for mozjpeg (Compress JPEG images) image-pngquant: Imagemin plugin for pngquant (Compress PNG images)

Dependency chain for these optimizers is as follow: imagemin-gifsicle > gifsicle > bin-wrapper > bin-version-check > bin-version > find-versions > semver-regex imagemin-mozjpeg > mozjpeg > bin-wrapper > bin-version-check > bin-version > find-versions > semver-regex iimagemin-pngquant> pngquant-bin > bin-wrapper > bin-version-check > bin-version > find-versions > semver-regex

All of them have dependency on bin-wrapper. The latest version of bin-wrapper is using bin-version-check: 4.0.0which is using semver-regex: 2.0. The issues lies with (latest version of) bin-wrapper using outdated bin-version-check. (bin-wrapper PR updating bin-version-check to latest version, bumping semver-regex to v4.

We can replace image-webpack-loader with image-minimizer-webpack-plugin, as long as we also move away from imagemin imagemin - optimize your images by default, since it is stable and works with all types of images squoosh - while working in experimental mode with .jpg, .jpeg, .png, .webp, .avif file types. sharp - High performance Node.js image processing, to resize and compress JPEG, PNG, WebP, AVIF and TIFF images. Uses the libvips library.

Since imagemin is depending on aforementioned plugins(gifsicle, mozjpeg, pngquant), we would have to consider squoosh or sharp in order to avoid ReDOS. In case of squoosh there seems to be no support for GIF.

arbrandes commented 1 year ago

@BilalQamar95, @jmbowman, are we moving forward with image-minimizer-webpack-plugin? For what it's worth, sharp looks nice and unbloated.

BilalQamar95 commented 1 year ago

Replaced image-webpack-loader with image-minimizer-webpack-plugin openedx/frontend-build#259

@arbrandes The shift would be required to resolve ReDOS, a High security vulnerability due to semver-regex. The PR for it is waiting on a re-review from Adam

arbrandes commented 1 year ago

@BilalQamar95, thanks! I hadn't noticed the PR existed.