Seagate / openSeaChest

Cross platform utilities useful for performing various operations on SATA, SAS, NVMe, and USB storage devices.
Other
489 stars 61 forks source link

Ownership of Released binaries is bad in Linux causing phantom "can't read file" errors #158

Open gdevenyi opened 3 days ago

gdevenyi commented 3 days ago

Trying to update a drive's firmware:

==========================================================================================
 openSeaChest_Firmware - openSeaChest drive utilities - NVMe Enabled
 Copyright (c) 2014-2024 Seagate Technology LLC and/or its Affiliates, All Rights Reserved
 openSeaChest_Firmware Version: 4.2.0-8_0_1 X86_64
 Build Date: Sep 25 2024
 Today: 20241014T004241 User: root
==========================================================================================
Sending SCSI Test Unit Ready

  CDB:

        0  1  2  3  4  5
  0x00 00 00 00 00 00 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (ms): 196.17

Test Unit Ready returning: SUCCESS

Sending SCSI Inquiry

  CDB:

        0  1  2  3  4  5
  0x00 12 00 00 00 60 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (ms): 51.30

Inquiry returning: SUCCESS

Sending SCSI Report LUNs

  CDB:

        0  1  2  3  4  5  6  7  8  9  A  B
  0x00 A0 00 00 00 00 00 00 00 00 10 00 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 262.39

Report LUNs returning: SUCCESS

Sending SCSI Inquiry, VPD = 00h

  CDB:

        0  1  2  3  4  5
  0x00 12 01 00 00 60 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 286.29

Inquiry returning: SUCCESS

Sending SCSI Inquiry, VPD = 80h

  CDB:

        0  1  2  3  4  5
  0x00 12 01 80 00 18 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 300.72

Inquiry returning: SUCCESS

Sending SCSI Inquiry, VPD = 83h

  CDB:

        0  1  2  3  4  5
  0x00 12 01 83 00 60 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 311.23

Inquiry returning: SUCCESS

Sending SCSI Inquiry, VPD = B1h

  CDB:

        0  1  2  3  4  5
  0x00 12 01 B1 00 40 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 285.18

Inquiry returning: SUCCESS

Sending SCSI Read Capacity 10 command

  CDB:

        0  1  2  3  4  5  6  7  8  9
  0x00 25 00 00 00 00 00 00 00 00 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 247.58

Read Capacity 10 returning: SUCCESS

Sending SCSI Read Capacity 16 command

  CDB:

        0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F
  0x00 9E 10 00 00 00 00 00 00 00 00 00 00 00 20 00 00

SG IO Issued as Indirect IO

  Sense Data Buffer:

        0  1  2  3  4  5  6  7
  0x00 00 00 00 00 00 00 00 00

Sense Key: 0h = No Error
ASC & ASCQ: 0h - 0h = No Additional Sense Information
FRU: 0h = No Additional Information
Information: 0000000000000000h
Command Specific Information: 0000000000000000h
Command Time (us): 254.72

Read Capacity 16 returning: SUCCESS

/dev/sg1 - ST32000444SS - 9WM5QLA3 - 0006 - SCSI
Couldn't open file firmware/MU-SAS-0008.LOD

The file definitely exists at the path and is accessible.

gdevenyi commented 3 days ago

After running the code through strace, I've found the issue:

readlink("firmware", 0x7fffac7a1f6f, 4080) = -1 EINVAL (Invalid argument)
readlink("firmware/MU-SAS-0008.LOD", 0x7fffac7a1f6f, 4096) = -1 EINVAL (Invalid argument)
getcwd("/root/openSeaChest-v24.08.1-linux-x86_64-portable", 4097) = 50
stat("firmware/MU-SAS-0008.LOD", {st_mode=S_IFREG|0644, st_size=1107456, ...}) = 0
stat("/root/openSeaChest-v24.08.1-linux-x86_64-portable/firmware/MU-SAS-0008.LOD", {st_mode=S_IFREG|0644, st_size=1107456, ...}) = 0
geteuid()                               = 0
lstat("/", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat("/root", {st_mode=S_IFDIR|0700, st_size=4096, ...}) = 0
lstat("/root/openSeaChest-v24.08.1-linux-x86_64-portable", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
writev(1, [{"Couldn't open file firmware/MU-S"..., 43}, {"\n", 1}], 2Couldn't open file firmware/MU-SAS-0008.LOD
) = 44
munmap(0x7fa432b16000, 16384)           = 0
close(3)                                = 0
madvise(0x7fa432b1e000, 4096, 0x8 /* MADV_??? */) = 0
munmap(0x7fa432b1d000, 32768)           = 0
writev(1, [{"", 0}, {"\n", 1}], 2
)      = 1
exit_group(3)                           = ?
+++ exited with 3 +++

The code to open the firmware appears to be attempting to compare the effective UID of the runtime to the ownership of all the file paths"

However the ownership of the published files from Releases have weird ownership:

root@server:~# ls -ldn openSeaChest-v24.08.1-linux-x86_64-portable
drwxr-xr-x 5 1001 999 4096 Oct 14 00:53 openSeaChest-v24.08.1-linux-x86_64-portable

After a chown root:root -R openSeaChest-v24.08.1-linux-x86_64-portable the command can read the firmware file.

vonericsen commented 3 days ago

Hi @gdevenyi,

Thanks for reporting this issue.

We implemented some Cert-C coding recommendations to check permissions and ownership before opening files. So that is where the checks are coming from. Here is some detail I added to the wiki about it.

The permissions/ownership are possibly like this due to how the Github Actions CI handles builds. I'll see if adding a step to set root:root before zipping it up will help.

gdevenyi commented 3 days ago

I figured this was intentional, but the release packages start off broken, a user should build software as an unprivileged user, and the error you get isn't very helpful.

All this adds up to a frustrating experience until I worked things out.

vonericsen commented 3 days ago

@gdevenyi,

That is totally fair and I also have on my todo list to expand on the reason for failing to open a file (permissions, ownership, file really not there, etc) as well since we introduced this change. I'll play with the CI and adding this step to see if that helps.

vonericsen commented 2 days ago

@gdevenyi,

As I'm looking into this, can you share some details about how you downloaded and extracted the release package so I can verify my changes are working as expected?

For example, did you do the download in Windows or Linux, was it moved to a system via USB, etc just so I can attempt to repeat the process and verify my change.

gdevenyi commented 2 days ago
vonericsen commented 2 days ago

@gdevenyi,

Thanks! That definitely repeated the issue.

When I tested the tar as my user rather than root, ownership was 1000:1000 (my username in place here), but when I run the tar command as root, I see the same weird ownership from the release.

I'm still testing to see if I can resolve this from the CI, but that is not what I would have expected to happen.

gdevenyi commented 2 days ago

When I tested the tar as my user rather than root, ownership was 1000:1000 (my username in place here)

That still ends up being a problem of course, because if you were to sudo ./openSeaChest_Firmware <etc> you would run into the same permissions issues having the firmware file locally as a user.

vonericsen commented 2 days ago

That still ends up being a problem of course, because if you were to sudo ./openSeaChest_Firmware you would run into the same permissions issues having the firmware file locally as a user.

I have not had this be an issue when I've tested it this way. In the permissions evaluation it allows files from the current user or root. With Sudo it will check if it's owned by root first, but I added a special case to determine if sudo was used to execute the tool, and if it was, to allow anything owned by that user to be trusted, so this should be ok in this case. The sudo check uses the environment variable SUDO_UID to figure out which user executed the command to determine if it is trusted or not. The idea in this case is the system administrator has already trusted a user with sudo privileges so it should be trusted in this case too....and it makes the software much easier to use in this case.

I'm still debugging to see if I can figure out why un-taring with root causes weird ownership to be set or how to resolve that.

gdevenyi commented 2 days ago

Unfortunately, the sudo bits don't work.

$ wget https://github.com/Seagate/openSeaChest/releases/download/v24.08.1/openSeaChest-v24.08.1-linux-x86_64-portable.tar.xz
$ tar xaf https://github.com/Seagate/openSeaChest/releases/download/v24.08.1/openSeaChest-v24.08.1-linux-x86_64-portable.tar.xz
$ unzip ConstellationES1-Muskie-SAS-StdOEM-0008.zip
$ sudo ./openSeaChest-v24.08.1-linux-x86_64-portable/openSeaChest_Firmware -d /dev/sg0 --downloadFW firmware/MU-SAS-0008.LOD
==========================================================================================
 openSeaChest_Firmware - openSeaChest drive utilities - NVMe Enabled
 Copyright (c) 2014-2024 Seagate Technology LLC and/or its Affiliates, All Rights Reserved
 openSeaChest_Firmware Version: 4.2.0-8_0_1 X86_64
 Build Date: Sep 25 2024
 Today: 20241015T134312 User: root
==========================================================================================

/dev/sg0 - ST32000444SS - <REDACTED> - 0006 - SCSI
Couldn't open file firmware/MU-SAS-0008.LOD
vonericsen commented 2 days ago

@gdevenyi,

I have now tried setting ownership before tar and after tar and I have not been able to get the ownership to set as root:root. Here is the before tar step in the CI and here is the after before it is uploaded

I do not know what else I can do to resolve this particular problem of ownership from the CI. If you have any other ideas for me to try I am happy to test them out as well. I'm only seeing this when I'm logged in as root for some reason, but my current user is working as expected with ownership being applied to that user.

Unfortunately, the sudo bits don't work

I will debug this some more but I know we have tested this to make sure it can accept user files and root files. It's possible we missed something during testing or someone "fixed" this with a chown or chmod and we didn't log it, but I will check to make sure this is working. The full error output I mentioned previously will also be helpful so I'll work on adding that in as well.

vonericsen commented 2 days ago

I figured out the tar issue!

I'm cleaning up my CI changes, but I have managed to get it to keep root:root when extracting as root and it keeps the current user when extracting as current user (say UID 1000).

What I was missing is -R on my chown command. What I learned (and did not know earlier) is that running tar as root by default preserves existing ownership of the contents unless you pass some other flags to tell it otherwise (you can pass --owner:root --group:root when extracting). So the user and group of the CI is what it was spitting out by default. Since these flags are GNU specific and I don't think many people know about them, I'll change the packaging steps to make sure everything is owned by root first so that this is not an issue for anyone else pulling down the files since I know many people will do this as root and it will throw them off.

vonericsen commented 2 days ago

@gdevenyi,

Can you please try using wget on this test build to confirm that the CI changes I have made work for you now? Testing linux tar ownership

gdevenyi commented 1 day ago

Running download/extract/commands as root work correctly now, no file access issues.