From dcf9e392c88efdbed6db602f91b79de72c052ec9 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Mon, 14 Aug 2023 18:26:17 -0700 Subject: [PATCH] Fix problems with `Assembler::ReflectionNotation()` Do more parameter validation, correct buffer size calculations for security-enhanced CRT functions, and avoid memcpy buffer overrun of locale. --- src/coreclr/ilasm/assem.cpp | 84 ++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/src/coreclr/ilasm/assem.cpp b/src/coreclr/ilasm/assem.cpp index dd2c91ac093acb..2bd90fadb8f916 100644 --- a/src/coreclr/ilasm/assem.cpp +++ b/src/coreclr/ilasm/assem.cpp @@ -1324,7 +1324,12 @@ OPCODE Assembler::DecodeOpcode(const BYTE *pCode, DWORD *pdwLen) char* Assembler::ReflectionNotation(mdToken tk) { + // We break the global static `wzUniBuf` into 2 equal parts: the first part is used for a Unicode + // string, the second part is used for a converted-into-multibyte (MB) string. Note that the MB string + // length is in bytes. char *sz = (char*)&wzUniBuf[dwUniBuf>>1], *pc; + const size_t szSizeBytes = (dwUniBuf * sizeof(WCHAR)) / 2; // sizeof(WCHAR) is 2, so this is just `dwUniBuf` + const size_t cchUniBuf = dwUniBuf / 2; // only use the first 1/2 of wzUniBuf *sz=0; switch(TypeFromToken(tk)) { @@ -1333,7 +1338,7 @@ char* Assembler::ReflectionNotation(mdToken tk) Class *pClass = m_lstClass.PEEK(RidFromToken(tk)-1); if(pClass) { - strcpy_s(sz,dwUniBuf>>1,pClass->m_szFQN); + strcpy_s(sz,szSizeBytes,pClass->m_szFQN); pc = sz; while((pc = strchr(pc,NESTING_SEP)) != NULL) { @@ -1348,31 +1353,80 @@ char* Assembler::ReflectionNotation(mdToken tk) { ULONG N; mdToken tkResScope; - if(SUCCEEDED(m_pImporter->GetTypeRefProps(tk,&tkResScope,wzUniBuf,dwUniBuf>>1,&N))) + if(SUCCEEDED(m_pImporter->GetTypeRefProps(tk,&tkResScope,wzUniBuf,cchUniBuf,&N))) { - WszWideCharToMultiByte(CP_UTF8,0,wzUniBuf,-1,sz,dwUniBuf>>1,NULL,NULL); + int ret = WszWideCharToMultiByte(CP_UTF8,0,wzUniBuf,-1,sz,szSizeBytes,NULL,NULL); if(TypeFromToken(tkResScope)==mdtAssemblyRef) { AsmManAssembly *pAsmRef = m_pManifest->m_AsmRefLst.PEEK(RidFromToken(tkResScope)-1); if(pAsmRef) { - pc = &sz[strlen(sz)]; - pc+=sprintf_s(pc,(dwUniBuf >> 1),", %s, Version=%d.%d.%d.%d, Culture=",pAsmRef->szName, + // We assume below that if sprintf_s fails due to buffer overrun, + // execution fails fast and sprintf_s doesn't return. + int sprintf_ret; + const size_t szLen = strlen(sz); + pc = &sz[szLen]; + size_t szRemainingSizeBytes = szSizeBytes - szLen; + + sprintf_ret = sprintf_s(pc,szRemainingSizeBytes,", %s, Version=%d.%d.%d.%d, Culture=",pAsmRef->szName, pAsmRef->usVerMajor,pAsmRef->usVerMinor,pAsmRef->usBuild,pAsmRef->usRevision); - ULONG L=0; - if(pAsmRef->pLocale && (L=pAsmRef->pLocale->length())) + pc += sprintf_ret; + szRemainingSizeBytes -= (size_t)sprintf_ret; + + unsigned L=0; + if(pAsmRef->pLocale && ((L=pAsmRef->pLocale->length()) > 0)) + { + // L is in bytes and doesn't include the terminating null. + if (L > (cchUniBuf - 1) * sizeof(WCHAR)) + { + report->error("Locale too long (%d characters, %d allowed).\n",L / sizeof(WCHAR), cchUniBuf - 1); + *sz=0; + break; + } + else if (szRemainingSizeBytes == 0) + { + report->error("TypeRef too long.\n"); + *sz=0; + break; + } + + if (szRemainingSizeBytes > 0) + { + memcpy(wzUniBuf,pAsmRef->pLocale->ptr(),L); + wzUniBuf[L>>1] = 0; + ret = WszWideCharToMultiByte(CP_UTF8,0,wzUniBuf,-1,pc,(int)szRemainingSizeBytes,NULL,NULL); + if (ret <= 0) + { + report->error("Locale too long.\n"); + *sz=0; + break; + } + else + { + pc += ret; + szRemainingSizeBytes -= (size_t)ret; + } + } + } + else { - memcpy(wzUniBuf,pAsmRef->pLocale->ptr(),L); - wzUniBuf[L>>1] = 0; - WszWideCharToMultiByte(CP_UTF8,0,wzUniBuf,-1,pc,dwUniBuf>>1,NULL,NULL); + sprintf_ret = sprintf_s(pc,szRemainingSizeBytes,"neutral"); + pc += sprintf_ret; + szRemainingSizeBytes -= (size_t)sprintf_ret; } - else pc+=sprintf_s(pc,(dwUniBuf >> 1),"neutral"); - pc = &sz[strlen(sz)]; - if(pAsmRef->pPublicKeyToken && (L=pAsmRef->pPublicKeyToken->length())) + if(pAsmRef->pPublicKeyToken && ((L=pAsmRef->pPublicKeyToken->length()) > 0)) { - pc+=sprintf_s(pc,(dwUniBuf >> 1),", Publickeytoken="); + sprintf_ret = sprintf_s(pc,szRemainingSizeBytes,", Publickeytoken="); + pc += sprintf_ret; + szRemainingSizeBytes -= (size_t)sprintf_ret; + BYTE* pb = (BYTE*)(pAsmRef->pPublicKeyToken->ptr()); - for(N=0; N> 1),"%2.2x",*pb); + for(unsigned i=0; i