From b9ef2051c5affe597995df530a386baaa9807a9e Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Wed, 9 Jun 2010 13:55:14 -0400 Subject: [PATCH] Windows: Revise SMB QuerySecurityInfo for MS10-020 MS10-020 (http://support.microsoft.com/kb/980232) has caused many problems for implementors of SMB 1.0 servers and applications that call GetFileSecurity() without checking the return code to determine if the call succeeded. The gist of the vulnerability was that the SMB redirector would pass any buffer it received to the application regardless of whether or not it was valid. MS10-020 protects the applications by strictly validating the SMB response data structure and the data in the security descriptor that is returned. The problem for SMB 1.0 server implementors is that there have been at least three different protocol descriptions for NT_TRANSACT_QUERY_SECURITY_DESC published over the last decade and all of them are incomplete. Therefore, just about no one but Microsoft has an SMB 1.0 server implementation that produces the exact out that they are expecting to validate. The end result is that in an attempt to protect applications from crashing due to invalid input being passed in directly caused dozens of applications to crash by not returning any security descriptor data at all. Even when the applications didn't crash they might not have been able to save their data. Cisco WAAS and NetApp DataOnTap systems were most adversely affected and they have had CIFS protocol licenses for many many years. To fix OpenAFS here is what needed to be done: 1. Instead of returning a security descriptor that gives ownership to the NUL SID, give it to the Everyone SID and set the flag that states that everyone has full access. 2. Validate the input parameters. In particular, check to ensure that the SMB file descriptor is valid and the file has not been deleted. 3. Enforce the maximum output data and parameter counts. 4. Handle buffer overflow and buffertoosmall conditions in the manner that Microsoft expects them to be handled. In particular, note that the parameter data which is returned in the SMB Data Region is not counted in the Data Count. Even if MaxData is 0, we can still return parameters values as long as MaxParm is large enough. LICENSE MIT Change-Id: I95034bc6f24a282decc507edcffb93bc58b986be Reviewed-on: http://gerrit.openafs.org/2110 Tested-by: Jeffrey Altman Reviewed-by: Derrick Brashear Reviewed-by: Asanka Herath Reviewed-by: Jeffrey Altman --- src/WINNT/afsd/cm.h | 1 + src/WINNT/afsd/smb.c | 3 + src/WINNT/afsd/smb3.c | 158 +++++++++++++++++++++++++++++++++--------- 3 files changed, 129 insertions(+), 33 deletions(-) diff --git a/src/WINNT/afsd/cm.h b/src/WINNT/afsd/cm.h index d3ba42784..ae10f05ed 100644 --- a/src/WINNT/afsd/cm.h +++ b/src/WINNT/afsd/cm.h @@ -103,6 +103,7 @@ #define CM_ERROR_FORCE_DNS_LOOKUP (CM_ERROR_BASE+61) #define CM_ERROR_BADFORMAT (CM_ERROR_BASE+62) #define CM_ERROR_RPC_MOREDATA (CM_ERROR_BASE+63) +#define CM_ERROR_BUFFER_OVERFLOW (CM_ERROR_BASE+64) /* Used by cm_FollowMountPoint and cm_FindVolumeByName */ /* And as an index in cm_volume_t */ diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index 4130752bc..bd8fcd212 100644 --- a/src/WINNT/afsd/smb.c +++ b/src/WINNT/afsd/smb.c @@ -3145,6 +3145,9 @@ void smb_MapNTError(long code, unsigned long *NTStatusp) else if (code == CM_ERROR_BUFFERTOOSMALL) { NTStatus = 0xC0000023L; /* Buffer too small */ } + else if (code == CM_ERROR_BUFFER_OVERFLOW) { + NTStatus = 0x80000005L; /* Buffer overflow */ + } else if (code == CM_ERROR_AMBIGUOUS_FILENAME) { NTStatus = 0xC0000035L; /* Object name collision */ } diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c index b2a6592bf..86c6e7673 100644 --- a/src/WINNT/afsd/smb3.c +++ b/src/WINNT/afsd/smb3.c @@ -8871,88 +8871,180 @@ long smb_ReceiveNTTranNotifyChange(smb_vc_t *vcp, smb_packet_t *inp, return 0; } -unsigned char nullSecurityDesc[36] = { +unsigned char nullSecurityDesc[] = { 0x01, /* security descriptor revision */ 0x00, /* reserved, should be zero */ - 0x00, 0x80, /* security descriptor control; - * 0x8000 : self-relative format */ + 0x04, 0x80, /* security descriptor control; + * 0x0004 : null-DACL present - everyone has full access + * 0x8000 : relative format */ 0x14, 0x00, 0x00, 0x00, /* offset of owner SID */ - 0x1c, 0x00, 0x00, 0x00, /* offset of group SID */ + 0x20, 0x00, 0x00, 0x00, /* offset of group SID */ 0x00, 0x00, 0x00, 0x00, /* offset of DACL would go here */ 0x00, 0x00, 0x00, 0x00, /* offset of SACL would go here */ - 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - /* "null SID" owner SID */ - 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 - /* "null SID" group SID */ + 0x01, 0x01, 0x00, 0x00, /* "everyone SID" owner SID */ + 0x00, 0x00, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x00, + 0x01, 0x01, 0x00, 0x00, /* "everyone SID" owner SID */ + 0x00, 0x00, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x00 }; /* NT_TRANSACT_QUERY_SECURITY_DESC (SMB_COM_NT_TRANSACT) */ long smb_ReceiveNTTranQuerySecurityDesc(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp) { int parmOffset, parmCount, dataOffset, dataCount; + int totalParmCount, totalDataCount; int parmSlot; - int maxData; + int maxData, maxParm; + int inTotalParm, inTotalData; + int inParm, inData; + int inParmOffset, inDataOffset; char *outData; char *parmp; USHORT *sparmp; ULONG *lparmp; USHORT fid; ULONG securityInformation; + smb_fid_t *fidp; + long code = 0; + DWORD dwLength; - parmOffset = smb_GetSMBOffsetParm(inp, 11, 1) + /* + * For details on the meanings of the various + * SMB_COM_TRANSACTION fields, see sections 2.2.4.33 + * of http://msdn.microsoft.com/en-us/library/ee442092%28PROT.13%29.aspx + */ + + inTotalParm = smb_GetSMBOffsetParm(inp, 1, 1) + | (smb_GetSMBOffsetParm(inp, 2, 1) << 16); + + inTotalData = smb_GetSMBOffsetParm(inp, 3, 1) + | (smb_GetSMBOffsetParm(inp, 4, 1) << 16); + + maxParm = smb_GetSMBOffsetParm(inp, 5, 1) + | (smb_GetSMBOffsetParm(inp, 6, 1) << 16); + + maxData = smb_GetSMBOffsetParm(inp, 7, 1) + | (smb_GetSMBOffsetParm(inp, 8, 1) << 16); + + inParm = smb_GetSMBOffsetParm(inp, 9, 1) + | (smb_GetSMBOffsetParm(inp, 10, 1) << 16); + + inParmOffset = smb_GetSMBOffsetParm(inp, 11, 1) | (smb_GetSMBOffsetParm(inp, 12, 1) << 16); - parmp = inp->data + parmOffset; + + inData = smb_GetSMBOffsetParm(inp, 13, 1) + | (smb_GetSMBOffsetParm(inp, 14, 1) << 16); + + inDataOffset = smb_GetSMBOffsetParm(inp, 15, 1) + | (smb_GetSMBOffsetParm(inp, 16, 1) << 16); + + parmp = inp->data + inParmOffset; sparmp = (USHORT *) parmp; lparmp = (ULONG *) parmp; fid = sparmp[0]; securityInformation = lparmp[1]; - maxData = smb_GetSMBOffsetParm(inp, 7, 1) - | (smb_GetSMBOffsetParm(inp, 8, 1) << 16); + fidp = smb_FindFID(vcp, fid, 0); + if (!fidp) { + osi_Log2(smb_logp, "smb_ReceiveNTTranQuerySecurityDesc Unknown SMB Fid vcp 0x%p fid %d", + vcp, fid); + return CM_ERROR_BADFD; + } - if (maxData < 36) - dataCount = 0; - else - dataCount = 36; + lock_ObtainMutex(&fidp->mx); + if (fidp->scp && (fidp->scp->flags & CM_SCACHEFLAG_DELETED)) { + lock_ReleaseMutex(&fidp->mx); + smb_CloseFID(vcp, fidp, NULL, 0); + smb_ReleaseFID(fidp); + return CM_ERROR_NOSUCHFILE; + } + lock_ReleaseMutex(&fidp->mx); + + osi_Log4(smb_logp,"smb_ReceiveNTTranQuerySecurityDesc fidp 0x%p scp 0x%p file \"%S\" Info=0x%x", + fidp, fidp->scp, osi_LogSaveClientString(smb_logp, fidp->NTopen_wholepathp), + securityInformation); + + smb_ReleaseFID(fidp); + + if ( securityInformation & ~(OWNER_SECURITY_INFORMATION|GROUP_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION) ) + { + code = CM_ERROR_BAD_LEVEL; + goto done; + } + + dwLength = sizeof( nullSecurityDesc); + + totalDataCount = dwLength; + totalParmCount = 4; + + if (maxData >= totalDataCount) { + dataCount = totalDataCount; + parmCount = min(totalParmCount, maxParm); + } else if (maxParm >= totalParmCount) { + totalDataCount = dataCount = 0; + parmCount = totalParmCount; + } else { + totalDataCount = dataCount = 0; + totalParmCount = parmCount = 0; + } /* out parms */ parmOffset = 8*4 + 39; parmOffset += 1; /* pad to 4 */ - parmCount = 4; + dataOffset = parmOffset + parmCount; parmSlot = 1; outp->oddByte = 1; /* Total Parameter Count */ - smb_SetSMBParmLong(outp, parmSlot, parmCount); parmSlot += 2; + smb_SetSMBParmLong(outp, parmSlot, totalParmCount); parmSlot += 2; /* Total Data Count */ - smb_SetSMBParmLong(outp, parmSlot, dataCount); parmSlot += 2; + smb_SetSMBParmLong(outp, parmSlot, totalDataCount); parmSlot += 2; /* Parameter Count */ smb_SetSMBParmLong(outp, parmSlot, parmCount); parmSlot += 2; /* Parameter Offset */ - smb_SetSMBParmLong(outp, parmSlot, parmOffset); parmSlot += 2; + smb_SetSMBParmLong(outp, parmSlot, parmCount ? parmOffset : 0); parmSlot += 2; /* Parameter Displacement */ smb_SetSMBParmLong(outp, parmSlot, 0); parmSlot += 2; /* Data Count */ smb_SetSMBParmLong(outp, parmSlot, dataCount); parmSlot += 2; /* Data Offset */ - smb_SetSMBParmLong(outp, parmSlot, dataOffset); parmSlot += 2; + smb_SetSMBParmLong(outp, parmSlot, dataCount ? dataOffset : 0); parmSlot += 2; /* Data Displacement */ smb_SetSMBParmLong(outp, parmSlot, 0); parmSlot += 2; - smb_SetSMBParmByte(outp, parmSlot, 0); /* Setup Count */ - smb_SetSMBDataLength(outp, 1 + parmCount + dataCount); + /* Setup Count */ + smb_SetSMBParmByte(outp, parmSlot, 0); - outData = smb_GetSMBData(outp, NULL); - outData++; /* round to get to parmOffset */ - *((ULONG *)outData) = 36; outData += 4; /* length */ + if (parmCount == totalParmCount && dwLength == dataCount) { + smb_SetSMBDataLength(outp, 1 + parmCount + dataCount); - if (maxData >= 36) { - memcpy(outData, nullSecurityDesc, 36); - outData += 36; - return 0; - } else - return CM_ERROR_BUFFERTOOSMALL; + /* Data */ + outData = smb_GetSMBData(outp, NULL); + outData++; /* round to get to dataOffset */ + + *((ULONG *)outData) = dataCount; outData += 4; /* SD Length (4 bytes) */ + memcpy(outData, nullSecurityDesc, dataCount); + outData += dataCount; + + code = 0; + } else if (parmCount >= 4) { + smb_SetSMBDataLength(outp, 1 + parmCount); + + /* Data */ + outData = smb_GetSMBData(outp, NULL); + outData++; /* round to get to dataOffset */ + + *((ULONG *)outData) = dwLength; outData += 4; /* SD Length (4 bytes) */ + code = CM_ERROR_BUFFERTOOSMALL; + } else { + smb_SetSMBDataLength(outp, 0); + code = CM_ERROR_BUFFER_OVERFLOW; + } + + done: + return code; } /* SMB_COM_NT_TRANSACT -- 2.39.5