Skip to content
This repository has been archived by the owner on Jul 29, 2020. It is now read-only.

Exit upon invalid MTYP in CMAP box #34

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Exit upon invalid MTYP in CMAP box #34

merged 1 commit into from
Jun 30, 2020

Conversation

jubalh
Copy link
Member

@jubalh jubalh commented Jun 19, 2020

@jubalh jubalh requested a review from MaxKellermann June 19, 2020 10:12
@MaxKellermann
Copy link
Contributor

I don't quite understand. This doesn't check for NULL pointers. This just breaks from the loop and the function upon seeing the first invalid object. Previously, these invalid objects were ignored. Bailing out early may or may not be correct, but how does this prevent a NULL pointer dereference?

@MaxKellermann
Copy link
Contributor

@theta682 can you explain, please?

@jubalh
Copy link
Member Author

jubalh commented Jun 23, 2020

Now I'm confused about it too. But I remember I reviewed this before. But this time only just created the PR because I saw that we still had it for the old repo without checking. And don't remember what I thought the last time I reviewed.

The title might be misleading. But CVE-2018-19542 is fixed indeed through this.

@jubalh
Copy link
Member Author

jubalh commented Jun 24, 2020

I saw that @theta682 recently started to watch our repo :)
Hi there! Hoping you can find some time to help us understand this one here.

@jubalh jubalh mentioned this pull request Jun 29, 2020
Closed
@jubalh jubalh changed the title Fix CVE-2018-19542: Check for NULL pointer in jp2_decode Exit upon invalid MTYP in CMAP box Jun 30, 2020
@jubalh
Copy link
Member Author

jubalh commented Jun 30, 2020

@MaxKellermann I changed the message. Let's review so we can merge the last two PRs in case mdadams merges our changes into his repo.

@theta682 @jubalh
jp2_dec: exit upon invalid MTYP in CMAP box
31178dc 
Proposed by Timothy Lyanguzov as a fix for CVE-2018-19542 at
jasper-software/jasper#200.

It appears 8c4e995 fixes
CVE-2018-19542 and CVE-2017-5499.

We take this change anyway since it improves things although not
directly fixes the CVE as proposed.
@jubalh jubalh merged commit 31178dc into master Jun 30, 2020
@theta682
Copy link
Contributor

Sorry guys, I didn't read notifications. My change was intended to fix reported CVE. I don't know the code and I could not phrase the change properly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MaxKellermann MaxKellermann MaxKellermann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jubalh @MaxKellermann @theta682