This bug applies to 3.2.x and above. A POSIX open call uses a trans2 setpathinfo call, which is automatically denied on a share set read-only. However, a POSIX read-only open should be allowed here.I have a fix for this that should apply cleanly to 3.2.x and upwards. I'm in the process of testing it right now, and will upload for review shortly. Jeremy.
CC:ing Jeff and Steve as they need to be aware of this. Jeremy.
Created attachment 4222 [details] Patch under test. This is the current patch I'm testing. The change to smbd/trans2.c opens up SETFILEINFO calls to POSIX_OPEN only. The change to first smbd/open.c closes 2 holes that would have been exposed by allowing POSIX_OPENS on readonly shares, and their ability to set arbitrary flags permutations. The O_CREAT -> O_CREAT|O_EXCL change removes an illegal combination (O_EXCL without O_CREAT) that previously was being passed down to the open syscall. Jeremy.
Patch applied to master and 3.4, passes testing. I think this needs applying to 3.3 and 3.2 also. Karolin, Volker, Steve & Jeff please comment. Thanks, Jeremy.
I haven't hit this problem personally. The patch however looks reasonable (from what little I know of samba's innards).
Given the problem, it also looks like it's something reasonable to push to stable branches. I'll have to leave the more detailed technical review to those more familiar with the code.
the patch looks correct to me
To be honest, I'm not really happy with that patch. Using a setfileinfo call for a read-only open is just a broken protocol, and having to put some complicated code paths for security-relevant stuff in makes me feel very, very uneasy. There is no other way like a qfileinfo for the r/o variant of posix open? Volker
We can't change the protocol at this point. It is what it is, and setfileinfo is what is used for the open. The patch opens one info level on a read-only share, and the inability to create/write depends on the same protections in the open path that NTCreateX uses to protect read-only shares from modification. The only reason that the changes are needed to the open code path, is to protect against the flags that can be used via raw posix open that aren't allowed by NTCreateX. Its a very minimal patch to fix something that will cause a lot of complaints once the new Linux kernel code is released generally. Just read it again, it really isn't that bad :-). Jeremy.
Alright, I'll go through the changes line by line, to make sure you're happy with them. Inside trans2.c the only real change is here: + if (info_level != SMB_POSIX_PATH_OPEN) { which only returns NT_STATUS_ACCESS_DENIED automatically on a setfilepathinfo call if it's not a SMB_POSIX_PATH_OPEN info level. i.e. it opens setfilepathinfo to this info level on a read-only share. Note that no other changes are needed, as it's simply going into a standard code path. As an example, look inside the reply_open() function in smbd/reply.c. Note there are *no* checks here for a read-only share, the checks are all done inside the open code path. In open.c this is the main change: if (!CAN_WRITE(conn)) { /* It's a read-only share - fail if we wanted to write. */ - if(accmode != O_RDONLY) { + if(accmode != O_RDONLY || (flags & O_TRUNC) || (flags & O_APPEND)) { DEBUG(3,("Permission denied opening %s\n", path)); return NT_STATUS_ACCESS_DENIED; This closes the "modify filesystem" path to open requests with flags of O_TRUNC and O_APPEND, which are the only two flags which can't be explicitly passed down to the low level POSIX open via the NTCreateX codepath. POSIX open calls could pass those flags down, so this is why the extra check is needed here. All other flags that could modify the underlying filesystem are already taken care of as they could have been passed down via the existing NTCreateX codepaths (O_CREAT) for example. The change : - flags &= ~O_CREAT; - local_flags &= ~O_CREAT; + flags &= ~(O_CREAT|O_EXCL); + local_flags &= ~(O_CREAT|O_EXCL); simply prevents an illegal combination of flags being passed down (O_EXCL only has meaning if O_CREAT is set, and in the read-only share case we're removing the O_CREAT flag, so we should remove the O_EXCL flag also). Hope this helps explain the change and why I think it's a safe one for 3.3/3.2. Jeremy.
Ok, thanks for the explanation. I think it is safe, but I would still have preferred if we could have lived without that patch. But if it kills your home multimedia setup, this patch probably has a very high priority.... Volker
Ouch ! Touche :-). It's not simply my home multimedia setup, important though that is. Let's decide based on input from Steve and Jeff, as they're the client that will be affected by this. If they both vote yes then let's do it, if not, then let's not. Does that work for you ? Jeremy.
Sure. Probably that's now in too wide use to be changed overall? What distros are using an "affected" cifs.ko? Volker
This is the first report of this bug that I know of. As far as what distros...I have no idea. This is a recent addition so any kernels ~2.6.29-ish and up will be heavily using POSIX opens when they're available. If we aren't seeing this bug much now, it may just be because users are lagging a little bit on updating kernels. Then again, it may just be that r/o cifs shares are uncommon enough that few people will be affected even once they upgrade. I sort of doubt this however. Key developers who have a setup that's affected by a bug is usually a strong vote in favour of inclusion to me. I'd probably lean toward including the patch.
Yes, my feeling is that as kernels start including the new open code this will be a common error reported back to distros including the client. Read only shares are very common out there. Jeremy.
Patch pushed to v3-3-test and v3-2-test (already in v3-4-test). Will be included in in the following releases. Closing out bug report. Thanks!