Fix problems with Assembler::ReflectionNotation() (#90655)

Do more parameter validation, correct buffer size calculations for security-enhanced CRT functions, and avoid memcpy buffer overrun of locale. Co-authored-by: Bruce Forstall <brucefo@microsoft.com>
dotnet · Aug 16, 2023 · 243a5ae · 243a5ae
1 parent 4d44f2f
commit 243a5ae
Showing 1 changed file with 69 additions and 15 deletions.
diff --git 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<L; N++,pb++) pc+=sprintf_s(pc,(dwUniBuf >> 1),"%2.2x",*pb);
+                                for(unsigned i=0; i<L; i++,pb++)
+                                {
+                                    sprintf_ret = sprintf_s(pc,szRemainingSizeBytes,"%2.2x",*pb);
+                                    pc += sprintf_ret;
+                                    szRemainingSizeBytes -= (size_t)sprintf_ret;
+                                }
                             }
                         }
                     }