| Message ID | 20220319032012.46ezg2pxjlrsdlpq@cyberdelia |
|---|---|
| State | New |
| Headers | show |
| Series | Signature check for LOGOFF response | expand |
On 3/18/2022 11:20 PM, Enzo Matsumiya wrote: > Hi, > > The LOGOFF command response is not signed (=> signature is 0x0), but we > check it anyway, displaying "sign fail" errors in ring buffer. What server is returning this unsigned response? Assuming it's Windows, that is either a doc bug or (arguably) a server bug, and should be reported before deciding how to address it here. Tom. > As far as I checked, an explicit LOGOUT is only sent when tlink pruning > happens (i.e. TLINK_IDLE_EXPIRE expires), but we have a case of this > causing issues on production env. > > I didn't find LOGOFF being a signature check exception in MS-SMB2 rev64. > Relevant sections: > > 2.2.7 SMB2 LOGOFF Request > 2.2.8 SMB2 LOGOFF Response > 3.2.5.4 Receiving an SMB2 LOGOFF Response > 3.3.5.6 Receiving an SMB2 LOGOFF Request > > If this is implementation defined, maybe something like this could work? > (100% untested) > > --- a/fs/cifs/smb2transport.c > +++ b/fs/cifs/smb2transport.c > @@ -667,6 +667,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct > TCP_Server_Info *server) > if ((shdr->Command == SMB2_NEGOTIATE) || > (shdr->Command == SMB2_SESSION_SETUP) || > (shdr->Command == SMB2_OPLOCK_BREAK) || > + (shdr->Command == SMB2_LOGOFF) || > server->ignore_signature || > (!server->session_estab)) > return 0; > > Thoughts? > > > Enzo >
Hi Tom, On 03/19, Tom Talpey wrote: >What server is returning this unsigned response? Assuming it's Windows, >that is either a doc bug or (arguably) a server bug, and should be >reported before deciding how to address it here. It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong signatures on READ responses with STATUS_END_OF_FILE. Our tests against Windows Server 2019 showed it to behave ok, so it looks like something on NetApp side. Thanks anyway. Enzo
On 3/23/2022 1:29 PM, Enzo Matsumiya wrote: > Hi Tom, > > On 03/19, Tom Talpey wrote: >> What server is returning this unsigned response? Assuming it's Windows, >> that is either a doc bug or (arguably) a server bug, and should be >> reported before deciding how to address it here. > > It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong > signatures on READ responses with STATUS_END_OF_FILE. > > Our tests against Windows Server 2019 showed it to behave ok, so it > looks like something on NetApp side. In this case I don't think it is appropriate to apply the suggested patch. Allowing unsigned or invalidly signed responses will greatly reduce security. I'll be interested if NetApp provides any information. Tom.
On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote: >On 3/23/2022 1:29 PM, Enzo Matsumiya wrote: >>Hi Tom, >> >>On 03/19, Tom Talpey wrote: >>>What server is returning this unsigned response? Assuming it's Windows, >>>that is either a doc bug or (arguably) a server bug, and should be >>>reported before deciding how to address it here. >> >>It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong >>signatures on READ responses with STATUS_END_OF_FILE. >> >>Our tests against Windows Server 2019 showed it to behave ok, so it >>looks like something on NetApp side. > >In this case I don't think it is appropriate to apply the suggested >patch. Allowing unsigned or invalidly signed responses will greatly >reduce security. I'll be interested if NetApp provides any information. Welcome to our world :-). Doing: git log|grep -i NetApp|wc -l shows 32 instances (some are commit messages with NetApp in them two or more times so the number is probably a little smaller than 32) of commits in Samba especially to deal with NetApp bugs :-). That's a lot of client bugfixes :-).
On 3/24/2022 12:23 PM, Jeremy Allison wrote: > On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote: >> On 3/23/2022 1:29 PM, Enzo Matsumiya wrote: >>> Hi Tom, >>> >>> On 03/19, Tom Talpey wrote: >>>> What server is returning this unsigned response? Assuming it's Windows, >>>> that is either a doc bug or (arguably) a server bug, and should be >>>> reported before deciding how to address it here. >>> >>> It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong >>> signatures on READ responses with STATUS_END_OF_FILE. >>> >>> Our tests against Windows Server 2019 showed it to behave ok, so it >>> looks like something on NetApp side. >> >> In this case I don't think it is appropriate to apply the suggested >> patch. Allowing unsigned or invalidly signed responses will greatly >> reduce security. I'll be interested if NetApp provides any information. > > Welcome to our world :-). Doing: > > git log|grep -i NetApp|wc -l > > shows 32 instances (some are commit messages with NetApp in > them two or more times so the number is probably a little > smaller than 32) of commits in Samba especially to > deal with NetApp bugs :-). > > That's a lot of client bugfixes :-). Well, it could be argued that this is prolonging the bad behavior. The NFS client maintainer's approach is the opposite - if the server is violating the protocol, he holds the line and will not change the client. I know, I know, it's all in how each person sees it. :) Tom.
On Thu, Mar 24, 2022 at 02:48:22PM -0400, Tom Talpey wrote: >On 3/24/2022 12:23 PM, Jeremy Allison wrote: >>On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote: >>>On 3/23/2022 1:29 PM, Enzo Matsumiya wrote: >>>>Hi Tom, >>>> >>>>On 03/19, Tom Talpey wrote: >>>>>What server is returning this unsigned response? Assuming it's Windows, >>>>>that is either a doc bug or (arguably) a server bug, and should be >>>>>reported before deciding how to address it here. >>>> >>>>It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong >>>>signatures on READ responses with STATUS_END_OF_FILE. >>>> >>>>Our tests against Windows Server 2019 showed it to behave ok, so it >>>>looks like something on NetApp side. >>> >>>In this case I don't think it is appropriate to apply the suggested >>>patch. Allowing unsigned or invalidly signed responses will greatly >>>reduce security. I'll be interested if NetApp provides any information. >> >>Welcome to our world :-). Doing: >> >>git log|grep -i NetApp|wc -l >> >>shows 32 instances (some are commit messages with NetApp in >>them two or more times so the number is probably a little >>smaller than 32) of commits in Samba especially to >>deal with NetApp bugs :-). >> >>That's a lot of client bugfixes :-). > >Well, it could be argued that this is prolonging the bad behavior. >The NFS client maintainer's approach is the opposite - if the server >is violating the protocol, he holds the line and will not change >the client. I know, I know, it's all in how each person sees it. :) It's all a question of leverage. A Microsoft behavior defines the protocol, even if it's an inadvertant bug. For NetApp, people using cifsfs, libsmbclient or our client tools just want the damn thing to work. NetApp only (as far as I know) test against a Microsoft client, so we have zero leverage here. Sucks, but as I said, "Welcome to our world" :-).
--- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -667,6 +667,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) if ((shdr->Command == SMB2_NEGOTIATE) || (shdr->Command == SMB2_SESSION_SETUP) || (shdr->Command == SMB2_OPLOCK_BREAK) || + (shdr->Command == SMB2_LOGOFF) || server->ignore_signature || (!server->session_estab)) return 0;