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

Feature/dirty cow #139

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Feature/dirty cow #139

wants to merge 9 commits into from

Conversation

iRave
Copy link

@iRave iRave commented Nov 30, 2016

Add Dirty COW vulnerability test to the VTS App

I used CMake to build the C code for the test, however, I also added the the necessary entry inside the Android.mk you normally use to build your native libraries. I used CMake to make the code debuggable and because it's the recommended way of building native code in android-studio. Please feel free to remove this though.

Copy link
Contributor

@Fuzion24 Fuzion24 left a comment

Choose a reason for hiding this comment

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

Hey there. Thank you for your contribution. I added some comments pointing out some bugs. If you'd go ahead and fix those, I'd gladly accept the pull request.

Cheers,
Ryan

}
if (fstat(oldFile,&st_oldFile) == -1) {
LOGV("could not open %s", argv[0]);
return cantOpenFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

You leave oldfile open here.

}
if (fstat(newFile,&st_newFile) == -1) {
LOGV("could not open %s", argv[1]);
return cantOpenFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

oldfile and newfile leak

int newFile=open(argv[1],O_RDONLY);
if (newFile == -1) {
LOGV("could not open %s", argv[1]);
return cantOpenFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

oldfile leaks here


LOGV("size %d\n\n",size);

mem_arg.patch = malloc(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

This memory is never freed


memset(mem_arg.patch, 0, size);

mem_arg.unpatch = malloc(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also leaked

void * map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, oldFile, 0);
if (map == MAP_FAILED) {
LOGV("mmap");
return cantMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

oldfile/newfile leaked

Copy link
Contributor

Choose a reason for hiding this comment

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

also, there is no corresponding munmap

LOGV("open(\"/proc/self/mem\"");
for (i = 0; i < LOOP; i++) {
lseek(fd, (off_t) mem_arg->offset, SEEK_SET);
c += write(fd, p, mem_arg->patch_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

This thread loops and writes no matter whether the private mapping was written to or not. You should check after each write if the r/o mapping was written to and bail on this loop if that is the case.

pthread_create(&pth1, NULL, madviseThread, mem_arg);
pthread_create(&pth2, NULL, procselfmemThread, mem_arg);

pthread_join(pth1, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the procselfthread completes, you should kill the madvise thread and bail. success or no.

char dest[STRINGSIZE];
const char *argv[2];
jstring string[2];
argv[0] = dest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using two files and trying to copy one over the other. Why dont you just mmap one r/o file, and test writing to it with a static buffer of 0x41 or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

(just a suggestion) You could have java land open the file descriptor to the target readonly file and just pass that in rather than the file name.

 Add thread to check periodically whether or not the exploit was successful
 Adjust maximal attempt count
@iRave
Copy link
Author

iRave commented Dec 1, 2016

I fixed the bugs you pointed out. Thanks, those were horrible...
I also added another thread to check the status of the exploit. I tested the vulnerability check and it should work with the new values. Anyway, there is no way you can guarantee that the 'procselfMem' thread would have had the possibility to overwrite the file if the phone was vulnerable. It could also just happen that the constellation for the race condition never occurred. On a Nexus 5 with a fixed kernel, the test runs for about 4 seconds. On a vulnerable device the exploit triggers after 1000-4000 tries (50000 is the new limit) wich is almost instantly.
Feel free to let me know what you think about these values.

@ale5000-git
Copy link

@iRave: Any update?

@iRave
Copy link
Author

iRave commented Feb 21, 2017

@ale5000-git as I pointed out in my comment, I finished the implementation and fixed the issues @Fuzion24 pointed out. I tested the code with different devices and kernels and it works reliably. From my side, this should be good to merge.

File file;
file = new File(context.getFilesDir(), file1Name);
file.delete();
file = new File(context.getFilesDir(), file1Name);
Copy link

Choose a reason for hiding this comment

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

This should be file2Name I think (yeah, I know that this repo is pretty dead, but still 😉 )

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks =D

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.

5 participants