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

Add limits on sampling factors #106

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Add limits on sampling factors #106

merged 2 commits into from
Jun 3, 2022

Conversation

sohomdatta1
Copy link
Contributor

Invalid sampling factors can lead to a divide by zero error
which could eventually lead to a infinite loop.

Also fixed failing tests + add a new test for invalid sampling
factors.

fixes #105

Invalid sampling factors can lead to a divide by zero error
which could eventually lead to a infinite loop.

Also fixed failing tests + add a new test for invalid sampling
factors.

fixes #105
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks very much for the fix!

test/index.js Show resolved Hide resolved
lib/decoder.js Show resolved Hide resolved
lib/decoder.js Show resolved Hide resolved
test/index.js Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

Also fixed failing tests

There shouldn't be failing tests. Can you elaborate? https://github.com/jpeg-js/jpeg-js/runs/3080117536?check_suite_focus=true

@sohomdatta1
Copy link
Contributor Author

sohomdatta1 commented Jun 2, 2022

Also fixed failing tests

There shouldn't be failing tests. Can you elaborate? https://github.com/jpeg-js/jpeg-js/runs/3080117536?check_suite_focus=true

The max resolution and max memory tests kept hitting the sampling factor error (which I added in the current patch) before the respective errors that were being tested causing both of those tests to fail.

The changes wrt to the old tests just replace the old binary blobs with ones that do not hit the sampling factor error but rather only the specific errors being tested.

Wrt to the fix itself, I was basing it on how the libjpeg-turbo and libjpeg libraries handled this particular scenario:
https://github.com/libjpeg-turbo/libjpeg-turbo/blob/1f55ae7b0fa3acc348a630171617d0e56d922b68/jdinput.c#L64-L78

Based on their documentation, the sampling rate must be between 1 and 4 according to the JPEG standard (thus side-stepping the divide by zero issue).
https://github.com/libjpeg-turbo/libjpeg-turbo/blob/9abeff46d87bd201a952e276f3e4339556a403a3/libjpeg.txt#L1138-L1146

@patrickhulce
Copy link
Collaborator

Awesome thank you for the very clear explanations @sohomdatta1! 🙏

Would you mind adding the link to https://github.com/libjpeg-turbo/libjpeg-turbo/blob/9abeff46d87bd201a952e276f3e4339556a403a3/libjpeg.txt#L1138-L1146 along with our = 1 changes and we'll merge this 🎉

@sohomdatta1
Copy link
Contributor Author

Awesome thank you for the very clear explanations @sohomdatta1! pray

Would you mind adding the link to https://github.com/libjpeg-turbo/libjpeg-turbo/blob/9abeff46d87bd201a952e276f3e4339556a403a3/libjpeg.txt#L1138-L1146 along with our = 1 changes and we'll merge this tada

Made the change :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you so much :)

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

Successfully merging this pull request may close these issues.

jpeg-js DoS (infinite loop)
2 participants