-
Notifications
You must be signed in to change notification settings - Fork 360
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
Give better error message for dangerous symlinks #3175
Conversation
Like said in the ticket, EPERM is just different kind of confusing because root doesn't expect to be told "not permitted" for regular file operations. This needs a specific error message about our symlink rules to meaningfully communicate to users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3100 (comment)
Flagging request changes so this doesn't show up as green for merging.
Looks much better. I'd leave fsmOpenat() return value alone and instead add the rc as the last argument 'int *rcp' (args returning data are typically last in rpm), that should require fewer changes across the board. |
lib/rpmfi.c
Outdated
@@ -2426,7 +2426,7 @@ char * rpmfileStrerror(int rc) | |||
case RPMERR_DIGEST_MISMATCH: s = _("Digest mismatch"); break; | |||
case RPMERR_INTERNAL: s = _("Internal error"); break; | |||
case RPMERR_UNMAPPED_FILE: s = _("Archive file not in header"); break; | |||
case RPMERR_INVALID_SYMLINK: s = _("Invalid symlink"); break; | |||
case RPMERR_INVALID_SYMLINK: s = _("Invalid or unsave symlink"); break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/unsave/unsafe/ although we can probably make the error message more informative yet. The sole purpose of RPMERR_INVALID_SYMLINK is to represent the misowned symlink concept that doesn't exist on the OS level where broken symlinks can be sufficiently expressed with errno stuff, so we should try to explain that in the message. The existing unused "invalid symlink" message was just something I tossed in there to avoid getting stuck on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ... actually just "Unsafe symlink" should be quite self-explanatory, once you go and look at the actual path we report.
Well, I modeled it after Changed the error message, though. |
It's just that fsmOpen() is the odd man out, as far as APIs go 😅 |
It's a stylistic thing of course, either way works. But since there's no need to turn the API upside down, keeping the change minimal helps make the actual change far more obvious. It's not a big patch - humor me and do an alternative version that just adds the detail rc as a return argument. Then we can decide which looks like the better patch for this change. I commonly do 3-4 different versions of stuff like this to see what makes the gist of the change stand out best, considering backportability and all. |
RPM refuses to follow non root owned symlinks pointing to files owned by another user for security reasons. This case was lumped in with other issues resulting in us setting errno to ENOTDIR. This led to confusing as the symlink often indeed points at a directory. Change fsmOpenat to report back an error code and use rpmfilesErrorCodes from rpmarchive.h - which are already used in the surrounding code. Give meaningful error message when encountering unsave symlinks. Retire the abuse of errno for our own reporting. Resolves: rpm-software-management#3100
Here you go. |
I realized yesterday evening that my request didn't come out right at all, this is not about entertaining me, and it's not a question of I prefer black and you prefer blue but black is right just because it's my preference. Sorry about that. The intention was really to have you compare the two versions side-by-side, and maybe see why I was suggesting that for yourself. I've done this kind of stuff so much that I just see familiar patterns and go on a kind of autopilot, and so I guess my change requests can seem pretty arbitrary, when they're not. When I say "I'd leave the return value alone", here's the mostly unconscious thought process behind it broken up: The version of the patch changing the return value to something else did two things at once: one amounts to refactoring and the other one the actual fix for the error message. You don't combine multiple things into the same commit, so either you split it to separate commits or find another way. Sometimes a refactor is required and that's the end of that, but if it's more of a cleanup thing, then you can do it before or after the fix. Leaving refactoring after the fix produces more backportable results when that's relevant (such as in this case), but refactoring first may produce a neater fix for the actual issue. Here, the return value refactor is optional (more on that below), so I favor the path of smaller change and backportability. That doesn't in any way exclude a cleanup refactor if it still seems worth it, and often it is. For a new API the order of arguments is largely a matter of style and preference, but why I think it's counter-productive here is that fsmOpenat() is following a very common pattern and isn't bad in itself, it just needs this side-band to report a more detailed error message. Like errno is for system calls. For adding that side-band, adding an (optional) argument at the end of parameter list is the most obvious and least intrusive way by far. Now, I agree having a variable called "rc" in the argument list (as per my suggestion) looks weird and non-obvious for the caller, but that's actually just a matter of bad naming. Call it "errnop" and it'll be immediately clear that this is just a side-band for the more detailed error code to be consulted when the call itself returned a failure. This small patch isn't in itself worth all this talk, but since this is a bit of a recurring theme I hope this clears up what kind of thinking goes behind my change requests. |
@@ -2426,7 +2426,7 @@ char * rpmfileStrerror(int rc) | |||
case RPMERR_DIGEST_MISMATCH: s = _("Digest mismatch"); break; | |||
case RPMERR_INTERNAL: s = _("Internal error"); break; | |||
case RPMERR_UNMAPPED_FILE: s = _("Archive file not in header"); break; | |||
case RPMERR_INVALID_SYMLINK: s = _("Invalid symlink"); break; | |||
case RPMERR_INVALID_SYMLINK: s = _("Unsave symlink"); break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Unsave/Unsafe/ again, also in the commit message
@@ -1027,7 +1038,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files, | |||
if (fp->suffix) | |||
flags |= AT_SYMLINK_NOFOLLOW; | |||
fd = fsmOpenat(di.dirfd, fp->fpath, flags, | |||
S_ISDIR(fp->sb.st_mode)); | |||
S_ISDIR(fp->sb.st_mode), &rc); | |||
if (fd < 0) | |||
rc = RPMERR_OPEN_FAILED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it should go inside fsmOpenat() now that it can.
Looking at the callsites and stuff in fsm a bit closer now, within fsm it's actually indeed fsmOpenat() that's the odd man out, everything there returns an rc as the main code and fsmOpen() just follows the general pattern whose existence I didn't even remember. So it's entirely possible remodeling fsmOpenat() after fsmOpen() would indeed make the whole saner. Feel free to add that as a separate refactoring commit. As a side note, amusingly there's exactly one caller of fsmOpen() and gets the return code checking wrong, probably my bad. |
I poked some more at this because the existing mess is largely my handiwork... It also brought back memories of wrestling with the fd/rc API mismatch when working on the symlink CVE stuff, it just didn't seem right no matter which way you made it. You were right though, making fsmOpenat() follow the common pattern within fsm makes the code a good deal saner when all the callers are updated to look at the rc instead of the fd/rc hodgepodge. Plus, there was some pretty stupid errror handling code in there. |
Merged as #3246 |
For all the blah blah here, there's one more thing that needs to be said: Apologies for what was a poor review from me. One cannot properly review a patch by looking at the patch alone, it always needs to be looked at in the real context. I think I'll remember that for a while. |
RPM refuses to follow non root owned symlinks pointing to files owned by another user for security reasons. This case was lumped in with other issues resulting in us setting errno to ENOTDIR. This led to confusing as the symlink often indeed points at a directory. Using EPERM is still not 100% right but points at least roughly into the right direction.
May be we should catch EPERM further up the call stack and use it to give a more meaningfull error message.
Resolves: #3100