fix(remove-system.map): skip read-only files instead of aborting

[maybebyte]

Summary

remove-system.map shreds System.map* files because they expose a kernel
symbol-to-address map useful for exploitation. The deletion loop aborts on
the first read-only match, so writable copies in other locations are left on
disk while the run still reports success.

Changes

• Root cause: the read-only branch of the loop calls exit 0, which
  terminates the whole script on the first non-writable System.map (commonly
  /boot, which is often read-only). Later writable copies under /usr/src,
  /lib/modules, and / are then never shredded, and the Done. Success.
  summary is skipped even though the exit code reports success.
• Replace exit 0 with continue so each read-only file is noted and the
  loop proceeds through the remaining locations. The exit code is unchanged.

Testing

• Minimal reproduction of the loop's control flow: with exit 0 the run stops
  after the first read-only entry (no summary printed); with continue it
  processes the following writable entry and prints Done. Success.
• shellcheck on the modified script: no new findings (only a pre-existing
  SC1091 info note on the unrelated source line).
• No automated tests cover this script.

Notes for reviewers

• Behavior delta: on systems where an early System.map (e.g. under /boot)
  is read-only, writable copies elsewhere that were previously skipped will now
  be shredded — the tool's intended behavior.

[adrelanos]

If the first match cannot be deleted what's the point of attempting to delete others?

[maybebyte]

@adrelanos Let me see if I understand what you're saying here. You're saying that, if even one System.map file cannot be removed, it is pointless to try to delete the other ones?

Since we are running this service as root, semantics of [ -w ...] mean that we are essentially checking if it is on a read-only filesystem or immutable, if I understand correctly.

My thought process was that it isn't necessarily better to abort and keep other copies around. Better to reduce the total number of info leaks, even if it's not perfect. I didn't want to make invasive changes to try and ensure purity on a first pass.

I also find it a little surprising on the user end. If I have a tool that removes System.map files, what I would expect as the contract is that it does its best to do that, rather than aborting early. Does that reasoning hold up from your side or am I missing something?

[ArrayBolt3]

I think the assumption was that if one file couldn't be deleted, surely the other ones couldn't be either, since the filesystem was read-only. This is because [ -w file ] on a read-only file as root states returns 0, i.e. the file is writable despite being "read-only" even to root. There is an exception though; if the file is immutable (chattr +i), then it will show as read-only even to root.

I can see the logic behind erasing everything we can rather than aborting, but I wonder if it would be worth using chattr -i on each file before attempting to delete. It would override a user who was trying to preserve the file, but if the user wanted to preserve the file that bad, they could just copy it somewhere outside of /boot. Doing a chattr +i on one of these files would possibly complicate kernel removal anyway, so it seems to me like these should never be immutable and thus removing the immutable bit if it is present would be the right way to go, in addition to doing this.

tl;dr: I'll probably merge this and add a chattr -i to things.

[adrelanos]

I think the assumption was that if one file couldn't be deleted, surely the other ones couldn't be either, since the filesystem was read-only.

Indeed. Because this only known to be happening in some App Qubes, where /lib/modules isn't writeable.

exit 0 -> continue makes little sense. If anything, the exit 0 could be removed without replacement. The effect is the same.

Old output (existing version):

INFO: Deleting system.map files...
NOTE: Cannot delete '/lib/modules/.../build/System.map' - read-only. For details, see: https://www.kicksecure.com/wiki/security-misc#system_map

New output (when using exit 0 -> continue or removing exit 0?) would become_

INFO: Deleting system.map files...
NOTE: Cannot delete '/lib/modules/.../build/System.map' - read-only. For details, see: https://www.kicksecure.com/wiki/security-misc#system_map
NOTE: Cannot delete '/lib/modules/.../source/System.map' - read-only. For details, see: https://www.kicksecure.com/wiki/security-misc#system_map
INFO: Done. Success.

So the output is worse.

That can be fixed but my thinking was it's more code that in fact doesn't effectively accomplish anything more.

[maybebyte]

Hey, thanks for the responses. I'm dealing with an NVMe SSD that is failing at the moment and am writing this off of an old spare computer that doesn't have all my dev tooling set up.

I think chattr -i makes sense. I believe a file with chattr +a would also cause [ -w ...] to return failure in these conditions, but that is pretty uncommon and I don't see a clear reason why a System.map only would be marked append-only.

Yeah, fair point about the continue being unneeded.

I was thinking in terms of the output that we could potentially collect failed files in an array and report them to the user at the end. Then link to the wiki once at the bottom. Or log one line for each failed attempt during iteration instead of storing in an array. Either way, I can squelch the success line since it is misleading. Open to thoughts about that.

I'd be willing to write the code for this once I've dealt with this other less fun stuff IRL, if it's something you all would be interested in. If not, that's cool too. I respect this project and want to help out.

[ArrayBolt3]

@maybebyte We're definitely interested in this :) Thanks for your submissions, they're quite helpful.