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

Incorrect integer handling CVE-2016-6252 #27

Closed
rbalint opened this issue Jul 24, 2016 · 6 comments
Closed

Incorrect integer handling CVE-2016-6252 #27

rbalint opened this issue Jul 24, 2016 · 6 comments

Comments

@rbalint
Copy link
Contributor

rbalint commented Jul 24, 2016

Reported to SuSe bug tracker with proposed fix.
https://bugzilla.suse.com/show_bug.cgi?id=979282

@rbalint
Copy link
Contributor Author

rbalint commented Jul 24, 2016

However using strtoul() in the fix does not really make a difference:

rbalint@chaos:~$ cat strtoul.c
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>

int main (int argc, char * argv[]) {
  unsigned long int i;
  errno = 0;
  i = (argc > 1) ? strtoul(argv[1], NULL, 0):0;
  printf("Read: %lu, errno: %d\n", i, errno);
  return 0;
}
rbalint@chaos:~$ gcc strtoul.c -o strtoul
rbalint@chaos:~$ ./strtoul -1
Read: 18446744073709551615, errno: 0
rbalint@chaos:~$ newuidmap $$ 0 10000 -1
newuidmap: uid range [0-18446744073709551615) -> [10000-9999) not allowed

@hallyn
Copy link
Member

hallyn commented Jul 30, 2016

Hi,

I'm not sure the testcase here means the fix won't work though. If you then check whether i < 100, you'll find that it isn't, meaning that verify_ranges() ought to do the right thing. it might be worth adding a "mapping->lower + mapping->count > mapping->lower" check.

@rbalint
Copy link
Contributor Author

rbalint commented Aug 18, 2016

I just wanted to show that strtoul() does not handle negative numbers as one (or at least I) would expect. The rest of the fix can still be good.

@hallyn
Copy link
Member

hallyn commented Dec 7, 2016

I believe this is now fixed.

@hallyn hallyn closed this as completed Dec 7, 2016
@jubalh
Copy link
Contributor

jubalh commented Sep 13, 2017

@hallyn which commit fixed it?

@jubalh
Copy link
Contributor

jubalh commented Sep 13, 2017

1d5a926 I guess.
Just mentioning for the record.

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

No branches or pull requests

3 participants