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

Fix an integer overflow issue #809

Merged
merged 5 commits into from
Sep 6, 2016
Merged

Fix an integer overflow issue #809

merged 5 commits into from
Sep 6, 2016

Conversation

trylab
Copy link
Contributor

@trylab trylab commented Aug 16, 2016

Prevent an integer overflow issue in function opj_pi_create_decode of
pi.c.

Hi, I found an integer overflow issue which could lead to OOB read and OOB write. I'd like to submit the proof-of-concept file and vulnerability report privately. Please give me an email address to report this issue. Thanks.

Regards,
Ke Liu of Tencent's Xuanwu LAB

Prevent an integer overflow issue in function opj_pi_create_decode of
pi.c.
l_current_pi->include = (OPJ_INT16*) opj_calloc((l_tcp->numlayers +1) * l_step_l, sizeof(OPJ_INT16));
/* prevent an integer overflow issue */
l_current_pi->include = 00;
if (l_step_l && l_tcp->numlayers < UINT_MAX / l_step_l - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

@trylab could you update the PR by replacing UINT_MAX by (1<<15)-1, hence avoiding inclusion of limits.h ? Thks

Copy link
Contributor

@stweil stweil Aug 21, 2016

Choose a reason for hiding this comment

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

Why do you want to avoid limits.h? IMHO using UINT_MAX is better than using an expression, especially if it is not correct (1 << 15 is a much too low limit)?

As opc_calloc takes size_t arguments, using stdint.h and SIZE_MAX would be even better:

if (l_step_l && l_tcp->numlayers < SIZE_MAX / l_step_l - 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi detonin and stweil, thanks for your reviewing. So which form is more preferable?
I agree that 1<<15 is too small. I'll change the code of this pull request once we come to a conclusion.

Making it more secure to call opj_calloc.
Remove header file limits.h
l_current_pi->include = (OPJ_INT16*) opj_calloc((l_tcp->numlayers +1) * l_step_l, sizeof(OPJ_INT16));
/* prevent an integer overflow issue */
l_current_pi->include = 00;
if (l_step_l && l_tcp->numlayers < ((OPJ_UINT32)-1) / sizeof(OPJ_INT16) / l_step_l - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

SIZE_MAX or ((size_t)-1) would be the real limit, because obj_calloc takes size_t parameters. On 64 bit platforms size_t is larger than OBJ_UINT32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infact using SIZE_MAX or ((size_t)-1) is wrong here. Please see the details at #825

Replace OPJ_UINT32 with SIZE_MAX
@trylab
Copy link
Contributor Author

trylab commented Aug 25, 2016

Thanks stweil. I'll use SIZE_MAX which was defined in opj_malloc.c.
I think there was something wrong with the travis build system so the check was not passed.

$ export CC=clang-3.9
$ clang-3.9 --version
/home/travis/build.sh: line 45: clang-3.9: command not found
......
./tools/travis-ci/run.sh: line 100: clang-3.9: command not found

BTW, the proof-of-concept file and report have been sent via email on 22nd Aug.

@detonin
Copy link
Contributor

detonin commented Aug 26, 2016

l_current_pi->include = (OPJ_INT16*) opj_calloc((l_tcp->numlayers +1) * l_step_l, sizeof(OPJ_INT16));
/* prevent an integer overflow issue */
l_current_pi->include = 00;
if (l_step_l && l_tcp->numlayers < ((SIZE_MAX)-1) / sizeof(OPJ_INT16) / l_step_l - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for my late comment.
The check with sizeof(OPJ_INT16) is unnecessary since it will be done by opj_calloc (through a call to calloc)
We can get rid of the check of l_step_l against 0 since l_tcp->numlayers + 1U > 0U and l_tcp->numlayers + 1U can't overflow.

The check can thus become:
if (l_step_l <= ((SIZE_MAX) / (l_tcp->numlayers + 1U)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've already updated the code.

@mayeut mayeut merged commit c16bc05 into uclouvain:master Sep 6, 2016
malaterre pushed a commit that referenced this pull request Sep 13, 2016
Prevent an integer overflow issue in function opj_pi_create_decode of
pi.c.
@detonin detonin added the bug label Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants