Give better error message for dangerous symlinks

[ffesti]

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

[pmatilai]

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.

[pmatilai]

See #3100 (comment)

Flagging request changes so this doesn't show up as green for merging.

[pmatilai]

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.

[pmatilai]

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.

[pmatilai]

So ... actually just "Unsafe symlink" should be quite self-explanatory, once you go and look at the actual path we report.

[ffesti]

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.

Well, I modeled it after static int fsmOpen(int *wfdp, int dirfd, const char *dest) to make the code more consistent. While the change might be a bit bigger, I think it's the better option. Also returning the fd and having the rc in the attrs seems weird to me.

Changed the error message, though.

[pmatilai]

It's just that fsmOpen() is the odd man out, as far as APIs go 😅
fsmOpenat() intentionally leans more towards openat(), which returns the fd and carries the success/error situation already, and just the more detailed error info is available through errno (and in my suggestion, rc parameter).

[pmatilai]

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.

[ffesti]

Here you go.

[pmatilai]

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.

[pmatilai]

s/Unsave/Unsafe/ again, also in the commit message

[pmatilai]

This looks like it should go inside fsmOpenat() now that it can.

[pmatilai]

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.

[pmatilai]

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.
Submitted #3246 based on this, just split up to separate commits, updated fsmOpenat() callers fully to the new style + additional cleanups.

[ffesti]

Merged as #3246

[pmatilai]

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.
Making an alternative version is often useful anyhow, but had I actually looked at the code instead of thinking I remember it, I wouldn't have asked for it in this case.