Merge pull request #11137 from keymanapp/fix/developer/11092-memory-b…

…uffer-overflow-in-compiler

fix(developer): handle buffer boundaries in four cases

developer/src/common/include/kmn_compiler_errors.h

@@ -175,6 +175,11 @@
 #define CERR_RepeatedBegin                                 0x00004073
 #define CERR_VirtualKeyInContext                           0x00004074
 
+#define CERR_OutsTooLong                                   0x00004075
+#define CERR_ExtendedStringTooLong                         0x00004076
+#define CERR_VirtualKeyExpansionTooLong                    0x00004077
+#define CERR_CharacterRangeTooLong                         0x00004078
+
 #define CWARN_TooManyWarnings                              0x00002080
 #define CWARN_OldVersion                                   0x00002081
 #define CWARN_BitmapNotUsed                                0x00002082

developer/src/kmc-kmn/src/compiler/kmn-compiler-messages.ts

@@ -559,6 +559,19 @@ export class KmnCompilerMessages {
   static ERROR_VirtualKeyInContext                            = SevError | 0x074;
   static Error_VirtualKeyInContext                            = () => m(this.ERROR_VirtualKeyInContext, `Virtual keys are not permitted in context`);
 
+  static ERROR_OutsTooLong                                    = SevError | 0x075;
+  static Error_OutsTooLong                                    = () => m(this.ERROR_OutsTooLong, `Store cannot be inserted with outs() as it makes the extended string too long`);
+
+  static ERROR_ExtendedStringTooLong                          = SevError | 0x076;
+  static Error_ExtendedStringTooLong                          = () => m(this.ERROR_ExtendedStringTooLong, `Extended string is too long`);
+
+  static ERROR_VirtualKeyExpansionTooLong                     = SevError | 0x077;
+  static Error_VirtualKeyExpansionTooLong                     = () => m(this.ERROR_VirtualKeyExpansionTooLong, `Virtual key expansion is too large`);
+
+  static ERROR_CharacterRangeTooLong                          = SevError | 0x078;
+  static Error_CharacterRangeTooLong                          = () => m(this.ERROR_CharacterRangeTooLong, `Character range is too large and cannot be expanded`);
+
+
   static WARN_TooManyWarnings                                 = SevWarn | 0x080;
   static Warn_TooManyWarnings                                 = () => m(this.WARN_TooManyWarnings, `Too many warnings or errors`);
 

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_character_range_too_long.kmn

@@ -0,0 +1,15 @@
+store(&NAME) 'error_character_range_too_long'
+store(&VERSION) '9.0'
+
+begin unicode > use(main)
+
+group(main) using keys
+
+c maximum store length is 4096 UTF-16 code units, including U+0000 terminator
+c #define GLOBAL_BUFSIZE  4096   // compfile.h
+c so we need 0x101E - 0x0020 + 1 = 0x0FFF --> 4095 words
+c See #11136 for calculation adjustment TODO
+
+store(x)    U+0020 .. U+101E
+
+any(x) + 'x' > 'x' context

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_extended_string_too_long.kmn

@@ -0,0 +1,16 @@
+store(&NAME) 'error_extended_string_too_long'
+store(&VERSION) '9.0'
+
+begin unicode > use(main)
+
+group(main) using keys
+
+c
+c maximum store length is 4096 UTF-16 code units, including U+0000 terminator
+c #define GLOBAL_BUFSIZE  4096   // compfile.h
+c so we need 0x101B - 0x0020 + 1 = 0x0FFD --> 4092 words, + 4 = 4096 = too long
+c See #11136 for calculation adjustment TODO
+
+store(x)    U+0020 .. U+101B
+
+outs(x) 'abcd' + 'x' > 'x' context

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_outs_too_long.kmn

@@ -0,0 +1,18 @@
+store(&NAME) 'error_outs_too_long'
+store(&VERSION) '9.0'
+
+begin unicode > use(main)
+
+group(main) using keys
+
+c maximum store length is 4096 UTF-16 code units, including U+0000 terminator
+c #define GLOBAL_BUFSIZE  4096   // compfile.h
+c so we need 0x101C - 0x0020 + 1 = 0x0FFD --> 4093 words
+c + 1, for 'a' in the rule below = 4094, which triggers the buffer boundary check.
+c Noting that this is conservative and losing 2 possible chars, but not fixing
+c in compiler.cpp at this time.
+c See #11136 for calculation adjustment TODO
+
+store(x)    U+0020 .. U+101C
+
+'a' outs(x) + 'x' > 'x' context

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_virtual_key_expansion_too_long.kmn

@@ -0,0 +1,17 @@
+store(&NAME) 'error_virtual_key_expansion_too_long'
+store(&VERSION) '9.0'
+
+begin unicode > use(main)
+
+group(main) using keys
+
+c maximum store length is 4096 UTF-16 code units, including U+0000 terminator
+c #define GLOBAL_BUFSIZE  4096   // compfile.h
+c so we need 0x101E - 0x0020 + 1 = 0x0FFF --> 4095 words
+c each vk is 5 words long UC_SENTINEL CODE_EXTENDED shift key CODE_EXTENDEDEND (some long history here!)
+c we start filling the buffer with 4066 words and then the remaining 30 bytes = 6 VKs A-F
+c See #11136 for calculation adjustment TODO
+
+store(x)    U+0020 .. U+1000 [K_A] .. [K_F]
+
+any(x) + 'x' > 'x' context

developer/src/kmc-kmn/test/test-messages.ts

@@ -94,4 +94,32 @@ describe('KmnCompilerMessages', function () {
     assert.equal(callbacks.messages[0].message, "Virtual keys are not supported in output");
   });
 
+  // ERROR_OutsTooLong
+
+  it('should generate ERROR_OutsTooLong if a store referenced in outs() is too long (more than GLOBAL_BUFSIZE elements)', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'error_outs_too_long.kmn'], KmnCompilerMessages.ERROR_OutsTooLong);
+    // callbacks.printMessages();
+  });
+
+  // ERROR_ExtendedStringTooLong
+
+  it('should generate ERROR_ExtendedStringTooLong if an extended string is too long (more than GLOBAL_BUFSIZE elements)', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'error_extended_string_too_long.kmn'], KmnCompilerMessages.ERROR_ExtendedStringTooLong);
+    // callbacks.printMessages();
+  });
+
+  // ERROR_VirtualKeyExpansionTooLong
+
+  it('should generate ERROR_VirtualKeyExpansionTooLong if a virtual key expansion is too long (more than GLOBAL_BUFSIZE elements)', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'error_virtual_key_expansion_too_long.kmn'], KmnCompilerMessages.ERROR_VirtualKeyExpansionTooLong);
+    // callbacks.printMessages();
+  });
+
+  // ERROR_CharacterRangeTooLong
+
+  it('should generate ERROR_CharacterRangeTooLong if a character range would expand to be too long (more than GLOBAL_BUFSIZE elements)', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'error_character_range_too_long.kmn'], KmnCompilerMessages.ERROR_CharacterRangeTooLong);
+    // callbacks.printMessages();
+  });
+
 });

developer/src/kmcmplib/src/CompMsg.cpp

@@ -111,6 +111,10 @@ const struct CompilerError CompilerErrors[] = {
     { CERR_DuplicateStore                                , "A store with this name has already been defined."},
     { CERR_RepeatedBegin                                 , "Begin has already been set"},
     { CERR_VirtualKeyInContext                           , "Virtual keys are not permitted in context"},
+    { CERR_OutsTooLong                                   , "Store cannot be inserted with outs() as it makes the extended string too long" },
+    { CERR_ExtendedStringTooLong                         , "Extended string is too long" },
+    { CERR_VirtualKeyExpansionTooLong                    , "Virtual key expansion is too large" },
+    { CERR_CharacterRangeTooLong                         , "Character range is too large and cannot be expanded" },
 
     { CHINT_UnreachableRule                              , "This rule will never be matched as another rule takes precedence"},
     { CHINT_NonUnicodeFile                               , "Keyman Developer has detected that the file has ANSI encoding. Consider converting this file to UTF-8"},

developer/src/kmcmplib/src/Compiler.cpp

@@ -1827,6 +1827,12 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
   p = str;
   do
   {
+    if (mx >= max) {
+      // This is an error condition, we want the compiler
+      // to crash if we reach this
+      return CERR_BufferOverflow;
+    }
+
     tokenFound = FALSE;
     while (iswspace(*p) && !u16chr(token, *p)) p++;
     if (!*p) break;
@@ -1905,7 +1911,7 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
     case 1:
       q = (PKMX_WCHAR) u16chr(p + 1, '\"');
       if (!q) return CERR_UnterminatedString;
-      if ((int)(q - p) - 1 + mx > max) return CERR_UnterminatedString;
+      if ((int)(q - p) - 1 + mx > max) return CERR_ExtendedStringTooLong;
       if (sFlag) return CERR_StringInVirtualKeySection;
       u16ncat(tstr,  p + 1, (int)(q - p) - 1);  // I3481
       mx += (int)(q - p) - 1;
@@ -1915,7 +1921,7 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
     case 2:
       q = (PKMX_WCHAR) u16chr(p + 1, '\'');
       if (!q) return CERR_UnterminatedString;
-      if ((int)(q - p) - 1 + mx > max) return CERR_UnterminatedString;
+      if ((int)(q - p) - 1 + mx > max) return CERR_ExtendedStringTooLong;
       if (sFlag) return CERR_StringInVirtualKeySection;
       u16ncat(tstr,  p + 1, (int)(q - p) - 1);  // I3481
       mx += (int)(q - p) - 1;
@@ -2031,7 +2037,9 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
       for (q = fk->dpStoreArray[i].dpString; *q; q++)
       {
         tstr[mx++] = *q;
-        if (mx >= max - 1) return CERR_BufferOverflow;
+        if (mx >= max - 1) {
+          return CERR_OutsTooLong;
+        }
       }
       tstr[mx] = 0;
       continue;
@@ -2421,7 +2429,6 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
       ErrChr = 0;
       return CERR_None;
     }
-    if (mx >= max) return CERR_BufferOverflow;
   } while (*p);
 
   if (!*token)
@@ -2630,7 +2637,9 @@ KMX_DWORD process_expansion(PFILE_KEYBOARD fk, PKMX_WCHAR q, PKMX_WCHAR tstr, in
       return CERR_ExpansionMustBePositive;
     }
     // Verify space in buffer
-    if (*mx + (HighKey - BaseKey) * 5 + 1 >= max) return CERR_BufferOverflow;
+    if (*mx + (HighKey - BaseKey) * 5 + 1 >= max) {
+      return CERR_VirtualKeyExpansionTooLong;
+    }
     // Inject an expansion.
     for (BaseKey++; BaseKey < HighKey; BaseKey++) {
       // < HighKey because caller will add HighKey to output
@@ -2657,12 +2666,16 @@ KMX_DWORD process_expansion(PFILE_KEYBOARD fk, PKMX_WCHAR q, PKMX_WCHAR tstr, in
       // < HighChar because caller will add HighChar to output
       if (Uni_IsSMP(BaseChar)) {
         // We'll test on each char to avoid complex calculations crossing SMP boundary
-        if (*mx + 3 >= max) return CERR_BufferOverflow;
+        if (*mx + 3 >= max) {
+          return CERR_CharacterRangeTooLong;
+        }
         tstr[(*mx)++] = (KMX_WCHAR) Uni_UTF32ToSurrogate1(BaseChar);
         tstr[(*mx)++] = (KMX_WCHAR) Uni_UTF32ToSurrogate2(BaseChar);
       }
       else {
-        if (*mx + 2 >= max) return CERR_BufferOverflow;
+        if (*mx + 2 >= max) {
+          return CERR_CharacterRangeTooLong;
+        }
         tstr[(*mx)++] = (KMX_WCHAR) BaseChar;
       }
     }