From 4e1176394a5ae50d2fd0b5790d9bff806dc08465 Mon Sep 17 00:00:00 2001 From: Steve Karg Date: Sat, 3 Jan 2026 15:11:34 -0600 Subject: [PATCH] Bugfix/ubasic-string-tokenizer-null-termination (#1196) * Fixed tokenizer_string() off-by-one buffer overflow when processing string literals longer than the buffer limit. * Fixed ubasic potential string buffer overflows by using snprintf instead of sprintf. * Fixed ubasic label strings to use UBASIC_LABEL_LEN_MAX as buffer limit. * Fixed ubasic string variables to initialize with zeros. * Fixed compile errors when UBASIC_DEBUG_STRINGVARIABLES is defined. * Added ubasic string variables user accessor API and unit testing for ubasic string variables. * Fixed tokenizer_label() off-by-one buffer overflow when processing string literals longer than the buffer limit. --- CHANGELOG.md | 10 ++- src/bacnet/basic/program/ubasic/tokenizer.c | 10 +-- src/bacnet/basic/program/ubasic/ubasic.c | 77 +++++++++++++++------ src/bacnet/basic/program/ubasic/ubasic.h | 2 + test/bacnet/basic/program/ubasic/src/main.c | 60 +++++++++++++++- 5 files changed, 131 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34d2e2d6..6386befe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,12 +12,20 @@ The git repositories are hosted at the following sites: * https://bacnet.sourceforge.net/ * https://github.com/bacnet-stack/bacnet-stack/ -## [Unreleased] - 2025-12-04 +## [Unreleased] - 2026-01-03 ### Security * Secured npdu_is_expected_reply() function where the MS/TP reply matcher could have an out-of-bounds read. (#1178) +* Secured ubasic interpreter tokenizer_string() and tokenizer_label() + off-by-one buffer overflow when processing string literals longer + than the buffer limit. + Fixed ubasic potential string buffer overflows by using snprintf. + Fixed ubasic label strings to use UBASIC_LABEL_LEN_MAX as buffer limit. + Fixed ubasic string variables to initialize with zeros. + Fixed compile errors when UBASIC_DEBUG_STRINGVARIABLES is defined. + Added ubasic string variables user accessor API and unit testing. (#1196) ### Added diff --git a/src/bacnet/basic/program/ubasic/tokenizer.c b/src/bacnet/basic/program/ubasic/tokenizer.c index 43232350..382b0ad8 100644 --- a/src/bacnet/basic/program/ubasic/tokenizer.c +++ b/src/bacnet/basic/program/ubasic/tokenizer.c @@ -556,8 +556,9 @@ void tokenizer_string(struct ubasic_tokenizer *tree, char *dest, uint8_t len) } while (*(string_end - 1) == '\\'); string_len = string_end - tree->ptr - 1; - if (len < string_len) { - string_len = len; + if (string_len > len - 1) { + /* space for null terminator */ + string_len = len - 1; } memcpy(dest, tree->ptr + 1, string_len); dest[string_len] = 0; @@ -586,8 +587,9 @@ void tokenizer_label(struct ubasic_tokenizer *tree, char *dest, uint8_t len) } break; } - if (string_len > len) { - string_len = len; + if (string_len > len - 1) { + /* space for null terminator */ + string_len = len - 1; } memcpy(dest, tree->ptr, string_len); dest[string_len] = 0; diff --git a/src/bacnet/basic/program/ubasic/ubasic.c b/src/bacnet/basic/program/ubasic/ubasic.c index 72307691..13ef4576 100644 --- a/src/bacnet/basic/program/ubasic/ubasic.c +++ b/src/bacnet/basic/program/ubasic/ubasic.c @@ -837,7 +837,9 @@ static int16_t sstr( *(data->stringstack + bp) = 0; bp++; - sprintf((char *)(data->stringstack + bp), "%ld", (long)j); + snprintf( + (char *)(data->stringstack + bp), sizeof(data->stringstack) - bp, "%ld", + (long)j); data->freebufptr = bp + strlen((char *)(data->stringstack + bp)) + 1; @@ -886,7 +888,7 @@ static int16_t sfactor(struct ubasic_data *data) { /* string form of factor */ int16_t r = 0, s = 0; - char tmpstring[UBASIC_STRINGLEN_MAX]; + char tmpstring[UBASIC_STRINGLEN_MAX] = { 0 }; UBASIC_VARIABLE_TYPE i, j; struct ubasic_tokenizer *tree = &data->tree; @@ -898,7 +900,7 @@ static int16_t sfactor(struct ubasic_data *data) break; case UBASIC_TOKENIZER_STRING: - tokenizer_string(tree, tmpstring, UBASIC_STRINGLEN_MAX); + tokenizer_string(tree, tmpstring, sizeof(tmpstring)); r = scpy(data, tmpstring); accept(data, UBASIC_TOKENIZER_STRING); break; @@ -1572,13 +1574,13 @@ static uint8_t jump_label(struct ubasic_data *data, char *label) static void gosub_statement(struct ubasic_data *data) { - char tmpstring[UBASIC_STRINGLEN_MAX]; + char tmplabel[UBASIC_LABEL_LEN_MAX] = { 0 }; struct ubasic_tokenizer *tree = &data->tree; accept(data, UBASIC_TOKENIZER_GOSUB); if (tokenizer_token(tree) == UBASIC_TOKENIZER_LABEL) { /* copy label */ - tokenizer_label(tree, tmpstring, UBASIC_STRINGLEN_MAX); + tokenizer_label(tree, tmplabel, sizeof(tmplabel)); tokenizer_next(tree); /* check for the end of line */ while (tokenizer_token(tree) == UBASIC_TOKENIZER_EOL) { @@ -1588,7 +1590,7 @@ static void gosub_statement(struct ubasic_data *data) data->gosub_stack[data->gosub_stack_ptr] = tokenizer_save_offset(tree); data->gosub_stack_ptr++; - jump_label(data, tmpstring); + jump_label(data, tmplabel); return; } } @@ -1616,15 +1618,15 @@ static void return_statement(struct ubasic_data *data) static void goto_statement(struct ubasic_data *data) { - char tmpstring[UBASIC_STRINGLEN_MAX]; + char tmplabel[UBASIC_LABEL_LEN_MAX] = { 0 }; struct ubasic_tokenizer *tree = &data->tree; accept(data, UBASIC_TOKENIZER_GOTO); if (tokenizer_token(tree) == UBASIC_TOKENIZER_LABEL) { - tokenizer_label(tree, tmpstring, sizeof(tmpstring)); + tokenizer_label(tree, tmplabel, sizeof(tmplabel)); tokenizer_next(tree); - jump_label(data, tmpstring); + jump_label(data, tmplabel); return; } @@ -1860,7 +1862,7 @@ static void bac_write_statement(struct ubasic_data *data) static void print_statement(struct ubasic_data *data, uint8_t println) { uint8_t print_how = 0; /*0-xp, 1-hex, 2-oct, 3-dec, 4-bin*/ - char tmpstring[UBASIC_STRINGLEN_MAX]; + char tmpstring[UBASIC_STRINGLEN_MAX] = { 0 }; struct ubasic_tokenizer *tree = &data->tree; /* string additions */ @@ -1879,30 +1881,37 @@ static void print_statement(struct ubasic_data *data, uint8_t println) } #if defined(UBASIC_VARIABLE_TYPE_STRING) if (tokenizer_token(tree) == UBASIC_TOKENIZER_STRING) { - tokenizer_string(tree, tmpstring, UBASIC_STRINGLEN_MAX); + tokenizer_string(tree, tmpstring, sizeof(tmpstring)); tokenizer_next(tree); } else #endif if (tokenizer_token(tree) == UBASIC_TOKENIZER_COMMA) { - sprintf(tmpstring, " "); + snprintf(tmpstring, sizeof(tmpstring), " "); tokenizer_next(tree); } else { #if defined(UBASIC_VARIABLE_TYPE_STRING) if (tokenizer_stringlookahead(tree)) { - sprintf(tmpstring, "%s", strptr(data, sexpr(data))); + snprintf( + tmpstring, sizeof(tmpstring), "%s", + strptr(data, sexpr(data))); } else #endif { if (print_how == 1) { - sprintf(tmpstring, "%lx", (unsigned long)relation(data)); + snprintf( + tmpstring, sizeof(tmpstring), "%lx", + (unsigned long)relation(data)); } else if (print_how == 2) { - sprintf(tmpstring, "%ld", (long)relation(data)); + snprintf( + tmpstring, sizeof(tmpstring), "%ld", + (long)relation(data)); } else { #if defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_24_8) || \ defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_22_10) fixedpt_str(relation(data), tmpstring, FIXEDPT_FBITS / 3); #else - sprintf(tmpstring, "%ld", relation(data)); + snprintf( + tmpstring, sizeof(tmpstring), "%ld", relation(data)); #endif } } @@ -2407,7 +2416,8 @@ static void serial_getline_completed(struct ubasic_data *data) #if defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_24_8) || \ defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_22_10) r = str_fixedpt( - data->statement, UBASIC_STRINGLEN_MAX, FIXEDPT_FBITS >> 1); + data->statement, sizeof(data->statement), + FIXEDPT_FBITS >> 1); #else r = atoi(data->statement); #endif @@ -3090,9 +3100,9 @@ void ubasic_set_stringvariable( #if defined(UBASIC_DEBUG_STRINGVARIABLES) serial_write_string(data, "set_stringvar:"); char msg[12]; - sprintf(msg, "[%d]", stringvariables[svarnum]); + snprintf(msg, sizeof(msg), "[%d]", data->stringvariables[svarnum]); serial_write_string(data, msg); - serial_write_string(data, strptr(stringvariables[svarnum])); + serial_write_string(data, strptr(data, data->stringvariables[svarnum])); serial_write_string(data, "\n"); #endif } @@ -3106,9 +3116,9 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum) #if defined(UBASIC_DEBUG_STRINGVARIABLES) serial_write_string(data, "get_stringvar:"); char msg[12]; - sprintf(msg, "[%d]", stringvariables[varnum]); + snprintf(msg, sizeof(msg), "[%d]", data->stringvariables[varnum]); serial_write_string(data, msg); - serial_write_string(data, strptr(stringvariables[varnum])); + serial_write_string(data, strptr(data, data->stringvariables[varnum])); serial_write_string(data, "\n"); #endif @@ -3117,6 +3127,31 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum) return (-1); } + +/** + * @brief Get pointer to string variable value. + * @param data ubasic data structure. + * @param variable string variable 'a' through 'z' or 'A' through 'Z'. + * @return Pointer to the string value, or NULL if variable is invalid. + */ +const char *ubasic_ptr_stringvariable(struct ubasic_data *data, char variable) +{ + uint8_t varnum; + + if (isupper(variable)) { + varnum = variable - 'A'; + } else if (islower(variable)) { + varnum = variable - 'a'; + } else { + return NULL; + } + if (varnum < UBASIC_STRING_VAR_LEN_MAX) { + return strptr(data, data->stringvariables[varnum]); + } + + return NULL; +} + /* */ /* end of string additions */ /* */ diff --git a/src/bacnet/basic/program/ubasic/ubasic.h b/src/bacnet/basic/program/ubasic/ubasic.h index 206ed803..a2d1cb82 100644 --- a/src/bacnet/basic/program/ubasic/ubasic.h +++ b/src/bacnet/basic/program/ubasic/ubasic.h @@ -270,6 +270,8 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum); BACNET_STACK_EXPORT void ubasic_set_stringvariable( struct ubasic_data *data, uint8_t varnum, int16_t size); +BACNET_STACK_EXPORT +const char *ubasic_ptr_stringvariable(struct ubasic_data *data, char variable); #endif /* API to interface and initialize the ported hardware drivers */ diff --git a/test/bacnet/basic/program/ubasic/src/main.c b/test/bacnet/basic/program/ubasic/src/main.c index 95bc8f62..0ba1db0f 100644 --- a/test/bacnet/basic/program/ubasic/src/main.c +++ b/test/bacnet/basic/program/ubasic/src/main.c @@ -383,6 +383,62 @@ static void test_ubasic_math(void) return; } +/** + * @brief Verify uBASIC string variable handling. + * @details Ensures that string variables are truncated at UBASIC_STRINGLEN_MAX + * and remain correctly null-terminated when assigned and read back. + */ +#if defined(CONFIG_ZTEST_NEW_API) +ZTEST(ubasic_tests, test_ubasic_strings) +#else +static void test_ubasic_strings(void) +#endif +{ + struct ubasic_data data = { 0 }; + const char *program1 = + /* program listing with either \0, \n, or ';' at the end of each line. + note: indentation is not required */ + "println 'Demo - Strings'\n" + "let a$ = \"1234567890123456789012345678901234567890X\"\n" + "print a$\n"; + const char *program2 = + /* program listing with either \0, \n, or ';' at the end of each line. + note: indentation is not required */ + "println 'Demo - Strings'\n" + "let z$ = \"1\"\n" + "print z$\n"; + const char *tmpstring; + + ubasic_load_program(&data, program1); + zassert_equal(data.status.bit.isRunning, 1, NULL); + zassert_equal(data.status.bit.Error, 0, NULL); + while (!ubasic_finished(&data)) { + ubasic_run_program(&data); + } + zassert_equal(data.status.bit.Error, 0, NULL); + tmpstring = ubasic_ptr_stringvariable(&data, 'a'); + zassert_not_null(tmpstring, "string variable not found"); + zassert_equal( + strlen(tmpstring), UBASIC_STRINGLEN_MAX - 1, "string length=%d", + (int)strlen(tmpstring)); + zassert_equal( + strcmp(tmpstring, "123456789012345678901234567890123456789"), 0, + "string value=%s", tmpstring); + + ubasic_load_program(&data, program2); + zassert_equal(data.status.bit.isRunning, 1, NULL); + zassert_equal(data.status.bit.Error, 0, NULL); + while (!ubasic_finished(&data)) { + ubasic_run_program(&data); + } + zassert_equal(data.status.bit.Error, 0, NULL); + tmpstring = ubasic_ptr_stringvariable(&data, 'z'); + zassert_not_null(tmpstring, "string variable not found"); + zassert_equal( + strlen(tmpstring), 1, "string length=%d", (int)strlen(tmpstring)); + zassert_equal(strcmp(tmpstring, "1"), 0, "string value=%s", tmpstring); +} + /** * @brief Test */ @@ -439,8 +495,8 @@ void test_main(void) { ztest_test_suite( ubasic_tests, ztest_unit_test(test_ubasic), - ztest_unit_test(test_ubasic_math), ztest_unit_test(test_ubasic_bacnet), - ztest_unit_test(test_ubasic_gpio)); + ztest_unit_test(test_ubasic_strings), ztest_unit_test(test_ubasic_math), + ztest_unit_test(test_ubasic_bacnet), ztest_unit_test(test_ubasic_gpio)); ztest_run_test_suite(ubasic_tests); }