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

added simple boot sector analyzer #6

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

added simple boot sector analyzer #6

wants to merge 6 commits into from

Conversation

rkgk04
Copy link
Collaborator

@rkgk04 rkgk04 commented Nov 22, 2017

this program can (1) check if the boot sector was compromised (by creating and comparing hash values with the backup boot sector at the end) and (2) scans and (currently) prints it onto the console.

issues:

  • currently no unit tests
  • might need to be edited/renamed to conform to naming conventions etc

@importantchoice
Copy link
Collaborator

Hey, cool!

Here some first thoughts about the code:

Some small code style related things that definitively should be changed before we merge something:

  • I saw, you use the print, without any (), so i guess you use python2? Our project should be python3 only and mixing python versions 2 and 3 would break the code.
  • docstrings of methods belong below the method signature, not above. Otherwise they won't be used by any autocompletion engine
  • pylint should tell the rest (like only 4 spaces for 1 level indentation, not 8 spaces)

But I have some questions too:

  • Did I understand it right, that you create an md5 hash over the 512 byte of the bootsector? If thats the case: why md5 and not a more secure thing like SHA256 or blake? I saw that FAT has a checksum field. Has NTFS maybe a similar bootsector entry and might that be a thing we should check for in this program? (just a thought)
  • @timbuntu could this code be integrated into our ntfs implementation?

And one last suggestion:

  • Use stream.tell() to save the beginning offset, to (1) restore the initial position after checking is done and (2) dont rely on the assumption, that the filesystem starts at byte 0

@timbuntu
Copy link
Collaborator

Yeah that is possible to be integrated, there already is a bootsector struct and you can get a struct of the main bootsector from the NTFS class.
So it wouldn't be a problem to also add the possibility to get the backup bootsector and then create a hash over both and compare them.

And about a checksum field in the bootsector:

It does theoretically have a checksum field, but I read something about it not being used, so I did just check and the value of the checksum field is indeed 0 .

@timbuntu
Copy link
Collaborator

timbuntu commented Nov 23, 2017

I added a method that returns a struct of the bootsector copy to the NTFS class now.
It returns a struct as defined in fishy/ntfs/ntfs_filesystem/bootsector.py in the ntfs-filesystem branch.

@importantchoice
Copy link
Collaborator

Any suggestions how/if we continue on this? As @timbuntu said, it might be easier to just read original bootsector and its copy and do a simple == comparison. As comparing 512 bytes for a one time check is not very resource intensive.

But you put some effort into this feature and I think this should be visible in the final submission. But I don't really know how we should do this if this piece of code will not be in our master... Suggestions welcome :)

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.

3 participants