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

Undefined behaviour in mprintf #5722

Closed
wants to merge 2 commits into from
Closed

Undefined behaviour in mprintf #5722

wants to merge 2 commits into from

Conversation

@stoeckmann
Copy link
Contributor

@stoeckmann stoeckmann commented Jul 25, 2020

Refactoring of mprintf code to prevent undefined behaviour (signed integer overflows and out of boundary accesses).

Verify that specified parameters are in range. If parameters are too
large, fail early on and avoid out of boundary accesses.

Also do not read behind boundaries of illegal format strings.

These are defensive measures since it is expected that format strings
are well-formed. Format strings should not be modifiable by user
input due to possible generic format string attacks.
@bagder
Copy link
Member

@bagder bagder commented Jul 25, 2020

Thanks, but this seems to make test 557 fail sometimes. example

@bagder
Copy link
Member

@bagder bagder commented Jul 26, 2020

BTW, it would be awesome if we could also add test cases that reproduce these problems and then these fixes. Can you see if you can add that to lib557.c ?

@bagder bagder added the crash label Jul 26, 2020
@stoeckmann
Copy link
Contributor Author

@stoeckmann stoeckmann commented Jul 26, 2020

I have added a test case for this.

Stack overflows can occur with precisions for integers and floats.

Proof of concepts:
- curl_mprintf("%d, %.*1$d", 500, 1);
- curl_mprintf("%d, %+0500.*1$f", 500, 1);

Ideally, compile with -fsanitize=address which makes this undefined
behavior a bit more defined for debug purposes.

The format strings are valid. The overflows occur due to invalid
arguments. If these arguments are variables with contents controlled
by an attacker, the function's stack can be corrupted.

Also see CVE-2016-9586 which partially fixed the float aspect.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
@bagder
bagder approved these changes Jul 26, 2020
@jay jay closed this in 94b0366 Jul 27, 2020
jay added a commit that referenced this pull request Jul 27, 2020
Stack overflows can occur with precisions for integers and floats.

Proof of concepts:
- curl_mprintf("%d, %.*1$d", 500, 1);
- curl_mprintf("%d, %+0500.*1$f", 500, 1);

Ideally, compile with -fsanitize=address which makes this undefined
behavior a bit more defined for debug purposes.

The format strings are valid. The overflows occur due to invalid
arguments. If these arguments are variables with contents controlled
by an attacker, the function's stack can be corrupted.

Also see CVE-2016-9586 which partially fixed the float aspect.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>

Closes #5722
@jay
Copy link
Member

@jay jay commented Jul 27, 2020

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.