samsung: Fix security vulnerabilities in vs-internal-log - #4016
Open
h1219-kim wants to merge 4 commits into
Open
samsung: Fix security vulnerabilities in vs-internal-log#4016h1219-kim wants to merge 4 commits into
h1219-kim wants to merge 4 commits into
Conversation
shr_valid_name() answers whether a string may be used as a name, but a caller holding a field that a device supplies -- a serial number, say -- needs to make one that may, not to turn the device away. Add that next to the validator so plugins share one definition of the character set rather than each carrying a copy of it. The two go through shr_name_char() now, so a sanitized string always satisfies shr_valid_name(). The micron plugin already carries a local sanitize_serial() that does the same thing; it can move over separately. Signed-off-by: Hyuntae Kim <h1219.kim@samsung.com>
vs-internal-log builds every dump file name from the controller serial number, which the drive supplies and nothing validates. A serial holding "../" makes make_file_path() return a path outside the directory -O asked for, and the dumps are opened with O_CREAT | O_TRUNC, so the drive picks which files get written. The one check on the serial refuses a "'" and lives in compress_dump_files(), so it runs only after the dumps are already on disk, and not at all without -z. Restrict the serial to the characters a file name may hold as soon as it is read from Identify Controller, before it reaches any path. Signed-off-by: Jinmin Hwang <jinmin.hwang@samsung.com> [h1219: split out of the combined fix, cover the path taken without -z] Signed-off-by: Hyuntae Kim <h1219.kim@samsung.com>
vs-internal-log built "tar" and "rm" command lines and handed them to system(). Every interpolated argument was single-quoted and an output path or serial number holding a "'" was refused, which covers /bin/sh but not cmd.exe, the shell system() runs on Windows: it does not treat "'" as a quoting character and keeps honouring &, |, > and ^, so a drive-supplied serial number could run arbitrary commands there. Spawn tar and rm through shr_spawnp() with a fixed argv, as the micron plugin does. No command string is built, so the "'" restriction goes away along with the shell and an output path may hold one again. Name the staged files for tar rather than passing ".": a "." operand also archives the staging directory itself as a "./" member, and extracting that member applies the staging directory's mode to whatever directory the archive is unpacked into. Signed-off-by: Jinmin Hwang <jinmin.hwang@samsung.com> [h1219: name the staged files for tar rather than ".", report the exit status instead of an errno, flush stdout before the spawn] Signed-off-by: Hyuntae Kim <h1219.kim@samsung.com>
vs-internal-log stages the dumps in a temp_samsung_dumps directory before archiving them. shr_mkdir_p() accepted a path that already existed, so a symlink, or a directory belonging to somebody else, sitting at that predictable name would collect the dumps instead; mode 0777 left the permissions to whatever the umask allowed. Create it with shr_mkdir() and mode 0700: a path that is already taken now fails rather than being reused, and the dumps stay unreadable to others for as long as collecting them takes. This covers the staging directory alone -- the archive it produces is the deliverable and keeps the mode tar gives it. The parent is created by the shr_mkdir_from_fname() that runs first, so dropping the "-p" costs nothing. Collecting every dump takes long enough that a run is interrupted now and then, and the staging directory outlives it, so the failure names the directory to remove rather than only reporting that the path is taken. Signed-off-by: Jinmin Hwang <jinmin.hwang@samsung.com> [h1219: record why shr_mkdir() replaces shr_mkdir_p(), name the directory to remove, document -z] Signed-off-by: Hyuntae Kim <h1219.kim@samsung.com>
h1219-kim
force-pushed
the
samsung-security-split
branch
from
September 10, 2026 19:39
99fc64c to
93e5228
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
shared: add shr_sanitize_name()comes first:shr_valid_name()only validates, and a caller holding a field the device supplies needs to make a usable name rather than turn the device away. It goes next to the validator instead of becoming a third copy of that character set — the micron plugin already carries a localsanitize_serial()that can move over separately.Then three fixes for
nvme samsung vs-internal-log, one issue per commit:../writes the dumps outside the directory-Oasked for. The old check refused a', ran only fromcompress_dump_files(), and so never ran without-z.tarandrmran throughsystem(). The single-quoting covers/bin/shbut not thecmd.exethatsystem()runs on Windows, where'is not a quoting character and&,|,>and^still apply. They now spawn throughshr_spawnp()with a fixed argv, as the micron plugin does.shr_mkdir_p()and mode 0777, so an existing symlink or somebody else's directory at that predictable name would collect the dumps. It is created withshr_mkdir()and mode 0700 instead, and a path that is already taken fails.tests/cli/nvme_samsung_test.pygrows from 40 to 51 cases, andshared/tests/test-string-util.ccovers the new helper.