search

TheMuso / smartmontools-xbox

Smartmontools with additions for locking/unlocking hard drives for use with the original XBox.
GNU General Public License v2.0
7 stars 4 forks source link

eeprom file name command line option is copied without a null terminator #1

Open MatthewTingum opened 3 years ago

MatthewTingum commented 3 years ago

This was first noticed when smartctl attempted to open a different file than the one I had provided on the command line.

Examples:

$ sudo ./smartctl -s security-eeprom-unlock,japan.bin /dev/sda
smartctl 7.0 (build date Apr  8 2021) [x86_64-linux-5.8.12-kAFL+] (local build)
Copyright (C) 2002-18, Bruce Allen, Christian Franke, www.smartmontools.org

Error - unable to open file japan.binn
$ sudo ./smartctl -s security-eeprom-unlock,j.bin /dev/sda
smartctl 7.0 (build date Apr  8 2021) [x86_64-linux-5.8.12-kAFL+] (local build)
Copyright (C) 2002-18, Bruce Allen, Christian Franke, www.smartmontools.org

Error - unable to open file j.binm.bin

These lines are the culprits: https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L931 https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L937 https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L941

The parsing here seems a little redundant. Maybe it can be cleaned up a bit.

It looks like the same bug shows up in the password parsing too: https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L955 https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L961 https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L967

Some options are inconsistent in their memset of set_security_password. https://github.com/TheMuso/smartmontools-xbox/blob/96500832cf4ce5f0ec92d1775094a05b4900bde2/smartmontools/smartctl.cpp#L966 Here and at least one other place, the length of the memset is 32. The buffer is 33 bytes in length and some options memset the entire 33 bytes. I think this value should be defined.

MatthewTingum commented 3 years ago

Looking at this from a maintainability perspective, I think the Xbox related functionality could be moved to it's own binary. We don't need to shoehorn this all in to smartctl. Admittedly, I haven't read through the ATA spec. I don't think locking and unlocking is part of S.M.A.R.T. SMART is supposed to be for diagnostics so why would it be? If it is, we should work towards adding support for unlocking drives to the smartctl project. Then we'd only need code to decrypt the keys.

I feel like the original authors hacked up this project because it has code to send ATA commands. Is there no well maintained library that can do this? If there is, I feel that we could create a new project based off of that. If there isn't, I still feel that separating drive unlocking / xbox related logic is best.