| Message ID | 20220917020704.25181-1-ematsumiya@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: verify signature only for valid responses | expand |
On 9/16/2022 10:07 PM, Enzo Matsumiya wrote: > The signature check will always fail for a response with SMB2 > Status == STATUS_END_OF_FILE, so skip the verification of those. Can you elaborate on this assertion? I don't see this as a protocol requirement: 3.2.5.1.3 Verifying the Signature The client MUST skip the processing in this section if any of the following is TRUE: - Client implements the SMB 3.x dialect family and decryption in section 3.2.5.1.1.1 succeeds - MessageId is 0xFFFFFFFFFFFFFFFF - Status in the SMB2 header is STATUS_PENDING [goes on to discuss action if session not found, etc] > Also, in async IO, it doesn't make sense to verify the signature > of an unsuccessful read (rdata->result != 0), as the data is > probably corrupt/inconsistent/incomplete. Verify only the responses > of successful reads. Same question. Why would we ever want to selectively skip signing verification? Signing protects against corrupted SMB headers, MITM, etc etc. Tom. > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> > --- > fs/cifs/smb2pdu.c | 4 ++-- > fs/cifs/smb2transport.c | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 6352ab32c7e7..9ae25ba909f5 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid) > case MID_RESPONSE_RECEIVED: > credits.value = le16_to_cpu(shdr->CreditRequest); > credits.instance = server->reconnect_instance; > - /* result already set, check signature */ > - if (server->sign && !mid->decrypted) { > + /* check signature only if read was successful */ > + if (server->sign && !mid->decrypted && rdata->result == 0) { > int rc; > > rc = smb2_verify_signature(&rqst, server); > diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c > index 1a5fc3314dbf..37c7ed2f1984 100644 > --- a/fs/cifs/smb2transport.c > +++ b/fs/cifs/smb2transport.c > @@ -668,6 +668,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->Status == STATUS_END_OF_FILE) || > server->ignore_signature || > (!server->session_estab)) > return 0;
On 09/17, Tom Talpey wrote: >On 9/16/2022 10:07 PM, Enzo Matsumiya wrote: >>The signature check will always fail for a response with SMB2 >>Status == STATUS_END_OF_FILE, so skip the verification of those. > >Can you elaborate on this assertion? I don't see this as a protocol >requirement: > > 3.2.5.1.3 Verifying the Signature > The client MUST skip the processing in this section if any of the > following is TRUE: > - Client implements the SMB 3.x dialect family and decryption in > section 3.2.5.1.1.1 succeeds > - MessageId is 0xFFFFFFFFFFFFFFFF > - Status in the SMB2 header is STATUS_PENDING > [goes on to discuss action if session not found, etc] Yeah I didn't find anything in the spec either. I woke up this morning thinking about this actually, and it might actually be a miscalculation on our side. My initial assumption, and debugging target now, is the 1-byte cropping done on some odd-sized structs, but I haven't deepened on that so far. I'll reply back with my findings later. >>Also, in async IO, it doesn't make sense to verify the signature >>of an unsuccessful read (rdata->result != 0), as the data is >>probably corrupt/inconsistent/incomplete. Verify only the responses >>of successful reads. > >Same question. Why would we ever want to selectively skip signing >verification? Signing protects against corrupted SMB headers, MITM, >etc etc. The problem here is actually different because rdata->result can contain an internal (kernel) error code when an underlying problem occurred (think EIO, EINTR, ECONNABORTED (not sure if possible this one), ENOMEM maybe?). But in between "mid set with MID_RESPONSE_RECEIVED state" and "verify the signature", the SMB2 header/message itself might be correct/valid, but our internal processing failed somewhere, so, the way I see it, computing the signature for such cases adds overhead and could (*) cover up the original internal error. (*) This actually brings to another inconsistency I'm currently looking at: switch (mid->mid_state) { case MID_RESPONSE_RECEIVED: credits.value = le16_to_cpu(shdr->CreditRequest); credits.instance = server->reconnect_instance; /* check signature only if read was successful */ if (server->sign && !mid->decrypted && rdata->result == 0) { rc is local >>>> int rc; rc = smb2_verify_signature(&rqst, server); if (rc) cifs_tcon_dbg(VFS, "SMB signature verification returned error = %d\n", rc); and never acted upon >>> } /* FIXME: should this be counted toward the initiating task? */ task_io_account_read(rdata->got_bytes); cifs_stats_bytes_read(tcon, rdata->got_bytes); break; } See, the return value of smb2_verify_signature() is never (aside from printing the error message) checked, used, or acted upon. So, even if server->ignore_signature is false (default), an invalid signature is never accounted for (regardless if the message is integral or a miscalculation on our side). Where, again IMO, the correct action would be to discard the mid and cancel the operation, as the data could be, intentionally or not, corrupted. Thoughts? >Tom. Cheers, Enzo >>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> >>--- >> fs/cifs/smb2pdu.c | 4 ++-- >> fs/cifs/smb2transport.c | 1 + >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >>diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>index 6352ab32c7e7..9ae25ba909f5 100644 >>--- a/fs/cifs/smb2pdu.c >>+++ b/fs/cifs/smb2pdu.c >>@@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid) >> case MID_RESPONSE_RECEIVED: >> credits.value = le16_to_cpu(shdr->CreditRequest); >> credits.instance = server->reconnect_instance; >>- /* result already set, check signature */ >>- if (server->sign && !mid->decrypted) { >>+ /* check signature only if read was successful */ >>+ if (server->sign && !mid->decrypted && rdata->result == 0) { >> int rc; >> rc = smb2_verify_signature(&rqst, server); >>diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c >>index 1a5fc3314dbf..37c7ed2f1984 100644 >>--- a/fs/cifs/smb2transport.c >>+++ b/fs/cifs/smb2transport.c >>@@ -668,6 +668,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->Status == STATUS_END_OF_FILE) || >> server->ignore_signature || >> (!server->session_estab)) >> return 0;
On 09/17, Enzo Matsumiya wrote: > So, even if >server->ignore_signature is false (default), Erm server->ignore_signature is checked in smb2_verify_signature() (obviously I was aware) and rc would be 0. I need coffee... But I stand by the rest of the argument. Enzo
On 9/17/2022 12:28 PM, Enzo Matsumiya wrote: > On 09/17, Tom Talpey wrote: >> On 9/16/2022 10:07 PM, Enzo Matsumiya wrote: >>> The signature check will always fail for a response with SMB2 >>> Status == STATUS_END_OF_FILE, so skip the verification of those. >> >> Can you elaborate on this assertion? I don't see this as a protocol >> requirement: >> >> 3.2.5.1.3 Verifying the Signature >> The client MUST skip the processing in this section if any of the >> following is TRUE: >> - Client implements the SMB 3.x dialect family and decryption in >> section 3.2.5.1.1.1 succeeds >> - MessageId is 0xFFFFFFFFFFFFFFFF >> - Status in the SMB2 header is STATUS_PENDING >> [goes on to discuss action if session not found, etc] > > Yeah I didn't find anything in the spec either. I woke up this morning > thinking about this actually, and it might actually be a miscalculation > on our side. My initial assumption, and debugging target now, is the > 1-byte cropping done on some odd-sized structs, but I haven't deepened > on that so far. > > I'll reply back with my findings later. Good, because there are definitely some tricky rules regarding what parts of the payload are included in the signing. Padding, especially, is easy to get wrong. >>> Also, in async IO, it doesn't make sense to verify the signature >>> of an unsuccessful read (rdata->result != 0), as the data is >>> probably corrupt/inconsistent/incomplete. Verify only the responses >>> of successful reads. >> >> Same question. Why would we ever want to selectively skip signing >> verification? Signing protects against corrupted SMB headers, MITM, >> etc etc. > > The problem here is actually different because rdata->result can > contain an internal (kernel) error code when an underlying problem > occurred (think EIO, EINTR, ECONNABORTED (not sure if possible this one), > ENOMEM maybe?). But in between "mid set with MID_RESPONSE_RECEIVED state" > and "verify the signature", the SMB2 header/message itself might be > correct/valid, but our internal processing failed somewhere, so, the Wait, we process the message *before* we check the signature??? Apart from inspecting the MID and verifying it's a response to a request we made, there isn't a lot to cause such an error. See 3.2.5.1.3. Also, if we process a bogus response, or drop a valid one, that's a seriously important issue. It's not a server/protocol/network error but we trashed the operation! Not quoting the ENOCOFFEE part of the rest of your message. :) Tom.
Hi Tom, On 09/17, Tom Talpey wrote: <snip> >Wait, we process the message *before* we check the signature??? Apart >from inspecting the MID and verifying it's a response to a request we >made, there isn't a lot to cause such an error. See 3.2.5.1.3. You're right. By processing I actually meant "parsing" done right after receive, but even that doesn't have many failure spots. I found that the mids with STATUS_END_OF_FILE are being discarded, apparently as per 3.2.5.11: If the Status field of the SMB2 header of the response indicates an error, the client MUST return the received status code to the calling application. What I found is that mid->callback() (smb2_readv_callback()) was being called from another thread, so even though the mid had been dequeued by mid->receive() earlier, smb2_readv_callback() was treating it as a valid (non-NULL), existing (mid_state == MID_RESPONSE_RECEIVED) mid. From this perspective, it makes sense to me to skip the signature verification when the mid wasn't supposed to be there in the first place, but if we consider that other messages with status != STATUS_SUCCESS have their signatures correctly computed (apparently), then I'd guess there's something wrong with computing signatures for STATUS_END_OF_FILE responses. Sent this just now: https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u I'd appreciate your, and Cc'd folks', feedback. Cheers, Enzo
On 09/18, Enzo Matsumiya wrote: > but if we consider that other messages with status != >STATUS_SUCCESS have their signatures correctly computed (apparently), >then I'd guess there's something wrong with computing signatures for >STATUS_END_OF_FILE responses. Correcting myself here: messages with status != STATUS_SUCCESS and != STATUS_END_OF_FILE take different path that never reaches ->calc_signature(). >Sent this just now: >https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u So I guess this patch is sufficient for now. Cheers, Enzo
On 9/18/2022 8:21 PM, Enzo Matsumiya wrote: > Hi Tom, > > On 09/17, Tom Talpey wrote: > <snip> >> Wait, we process the message *before* we check the signature??? Apart >> from inspecting the MID and verifying it's a response to a request we >> made, there isn't a lot to cause such an error. See 3.2.5.1.3. > > You're right. By processing I actually meant "parsing" done right after > receive, but even that doesn't have many failure spots. > > I found that the mids with STATUS_END_OF_FILE are being discarded, > apparently as per 3.2.5.11: > > If the Status field of the SMB2 header of the response indicates an > error, the client MUST return the received status code to the calling > application. I don't think it follows that the signature MUST NOT be validated. That processing is fundamental, and is independent of returning results. 3.2.5.1.3 has only three exceptions, and STATUS_END_OF_FILE isn't one of them. > What I found is that mid->callback() (smb2_readv_callback()) was being > called from another thread, so even though the mid had been dequeued by > mid->receive() earlier, smb2_readv_callback() was treating it as a > valid (non-NULL), existing (mid_state == MID_RESPONSE_RECEIVED) mid. That certainly sounds like another bug. Why are two threads processing the MID? Is this an async response? > From this perspective, it makes sense to me to skip the signature > verification when the mid wasn't supposed to be there in the first That's not what is documented. Only if the MID is 0xFFFFFFFFFFFFFFFF. > place, but if we consider that other messages with status != > STATUS_SUCCESS have their signatures correctly computed (apparently), > then I'd guess there's something wrong with computing signatures for > STATUS_END_OF_FILE responses. I agree. Tom. > Sent this just now: > https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u > > I'd appreciate your, and Cc'd folks', feedback. > > > Cheers, > > Enzo >
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 6352ab32c7e7..9ae25ba909f5 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid) case MID_RESPONSE_RECEIVED: credits.value = le16_to_cpu(shdr->CreditRequest); credits.instance = server->reconnect_instance; - /* result already set, check signature */ - if (server->sign && !mid->decrypted) { + /* check signature only if read was successful */ + if (server->sign && !mid->decrypted && rdata->result == 0) { int rc; rc = smb2_verify_signature(&rqst, server); diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c index 1a5fc3314dbf..37c7ed2f1984 100644 --- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -668,6 +668,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->Status == STATUS_END_OF_FILE) || server->ignore_signature || (!server->session_estab)) return 0;
The signature check will always fail for a response with SMB2 Status == STATUS_END_OF_FILE, so skip the verification of those. Also, in async IO, it doesn't make sense to verify the signature of an unsuccessful read (rdata->result != 0), as the data is probably corrupt/inconsistent/incomplete. Verify only the responses of successful reads. Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> --- fs/cifs/smb2pdu.c | 4 ++-- fs/cifs/smb2transport.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)