Unsolicited GNU bc patch
Greetings,
Yes, I keep spamming this trying to find an appropriate mailing list. I don't
remember how or why I initially stumbled across this bug report
(https://bugs.launchpad.net/ubuntu/+source/bc/+bug/1775776), but, given that
I have some familiarity with GNU bc, I decided to fix some of the issues.
Turns out, this also seems to fix the crashes reported here
(https://www.openwall.com/lists/oss-security/2018/11/28/1). I think it would
be a lot more useful to share this, as there isn't a lot to review. There are
three bug fixes and some self-defensive checks in the runtime for malformed
bytecode. Address Sanitizer tells me that these previously invalid memory
references now just leak memory. I don't appear to have broken anything in the
process, either. I'm not a member of any Debian mailing list, but I will try
to watch for responses.
Just trying to be somewhat helpful,
Thomas DiModica
From 3ecfe21c965956f3913e9bc340df729234e4453b Mon Sep 17 00:00:00 2001
From: Thomas DiModica <ricinwich@yahoo.com>
Date: Tue, 19 Jul 2022 19:28:12 -0600
Subject: [PATCH] Resolving the crashes found through fuzz testing by
HongxuChen.
---
bc/execute.c | 54 +++++++++++++++++++++++++++++++++-------------------
bc/storage.c | 38 ++++++++++++++++++++++++++++++++++--
bc/util.c | 2 +-
3 files changed, 71 insertions(+), 23 deletions(-)
diff --git a/bc/execute.c b/bc/execute.c
index 256e4b7..d30c6f5 100644
--- a/bc/execute.c
+++ b/bc/execute.c
@@ -130,7 +130,7 @@ execute (void)
gp = functions[pc.pc_func].f_label;
l_gp = label_num >> BC_LABEL_LOG;
l_off = label_num % BC_LABEL_GROUP;
- while (l_gp-- > 0) gp = gp->l_next;
+ while ((l_gp-- > 0) && (gp != NULL)) gp = gp->l_next;
if (gp)
pc.pc_addr = gp->l_adrs[l_off];
else {
@@ -146,6 +146,13 @@ execute (void)
if ((new_func & 0x80) != 0)
new_func = ((new_func & 0x7f) << 8) + byte(&pc);
+ /* Check to make sure it is valid. */
+ if (new_func >= f_count)
+ {
+ rt_error ("Internal error.");
+ break;
+ }
+
/* Check to make sure it is defined. */
if (!functions[new_func].f_defined)
{
@@ -204,25 +211,32 @@ execute (void)
case 'O' : /* Write a string to the output with processing. */
while ((ch = byte(&pc)) != '"')
- if (ch != '\\')
- out_schar (ch);
- else
- {
- ch = byte(&pc);
- if (ch == '"') break;
- switch (ch)
- {
- case 'a': out_schar (007); break;
- case 'b': out_schar ('\b'); break;
- case 'f': out_schar ('\f'); break;
- case 'n': out_schar ('\n'); break;
- case 'q': out_schar ('"'); break;
- case 'r': out_schar ('\r'); break;
- case 't': out_schar ('\t'); break;
- case '\\': out_schar ('\\'); break;
- default: break;
- }
- }
+ {
+ if (pc.pc_addr == functions[pc.pc_func].f_code_size)
+ {
+ rt_error ("Broken String.");
+ break;
+ }
+ if (ch != '\\')
+ out_schar (ch);
+ else
+ {
+ ch = byte(&pc);
+ if (ch == '"') break;
+ switch (ch)
+ {
+ case 'a': out_schar (007); break;
+ case 'b': out_schar ('\b'); break;
+ case 'f': out_schar ('\f'); break;
+ case 'n': out_schar ('\n'); break;
+ case 'q': out_schar ('"'); break;
+ case 'r': out_schar ('\r'); break;
+ case 't': out_schar ('\t'); break;
+ case '\\': out_schar ('\\'); break;
+ default: break;
+ }
+ }
+ }
fflush (stdout);
break;
diff --git a/bc/storage.c b/bc/storage.c
index c79db82..28e933b 100644
--- a/bc/storage.c
+++ b/bc/storage.c
@@ -349,6 +349,7 @@ get_var (int var_name)
{
var_ptr = variables[var_name] = bc_malloc (sizeof (bc_var));
bc_init_num (&var_ptr->v_value);
+ var_ptr->v_next = NULL;
}
return var_ptr;
}
@@ -370,6 +371,12 @@ get_array_num (int var_index, unsigned long idx)
unsigned int ix, ix1;
int sub [NODE_DEPTH];
+ if (var_index >= a_count)
+ {
+ rt_error ("Internal Error.");
+ return NULL;
+ }
+
/* Get the array entry. */
ary_ptr = arrays[var_index];
if (ary_ptr == NULL)
@@ -588,6 +595,12 @@ store_array (int var_name)
bc_num *num_ptr;
long idx;
+ if (var_name >= a_count)
+ {
+ rt_error ("Internal Error.");
+ return;
+ }
+
if (!check_stack(2)) return;
idx = bc_num2long (ex_stack->s_next->s_num);
if (idx < 0 || idx > BC_DIM_MAX ||
@@ -666,6 +679,12 @@ load_array (int var_name)
bc_num *num_ptr;
long idx;
+ if (var_name >= a_count)
+ {
+ rt_error ("Internal Error.");
+ return;
+ }
+
if (!check_stack(1)) return;
idx = bc_num2long (ex_stack->s_num);
if (idx < 0 || idx > BC_DIM_MAX ||
@@ -746,6 +765,12 @@ decr_array (int var_name)
bc_num *num_ptr;
long idx;
+ if (var_name >= a_count)
+ {
+ rt_error ("Internal Error.");
+ return;
+ }
+
/* It is an array variable. */
if (!check_stack (1)) return;
idx = bc_num2long (ex_stack->s_num);
@@ -828,6 +853,12 @@ incr_array (int var_name)
bc_num *num_ptr;
long idx;
+ if (var_name >= a_count)
+ {
+ rt_error ("Internal Error.");
+ return;
+ }
+
if (!check_stack (1)) return;
idx = bc_num2long (ex_stack->s_num);
if (idx < 0 || idx > BC_DIM_MAX ||
@@ -1018,7 +1049,11 @@ process_params (program_counter *progctr, int func)
/* Compute source index and make sure some structure exists. */
ix = (int) bc_num2long (ex_stack->s_num);
- (void) get_array_num (ix, 0);
+ if (get_array_num (ix, 0) == NULL)
+ {
+ rt_error ("Internal Error.");
+ return;
+ }
/* Push a new array and Compute Destination index */
auto_var (params->av_name);
@@ -1049,7 +1084,6 @@ process_params (program_counter *progctr, int func)
else
rt_error ("Parameter type mismatch, parameter %s.",
v_names[params->av_name]);
- params++;
}
pop ();
}
diff --git a/bc/util.c b/bc/util.c
index 8eba093..5ff93e0 100644
--- a/bc/util.c
+++ b/bc/util.c
@@ -610,7 +610,7 @@ lookup (char *name, int namekind)
{
if (id->v_name >= v_count)
more_variables ();
- v_names[id->v_name - 1] = name;
+ v_names[id->v_name] = name;
return (id->v_name);
}
yyerror ("Too many variables");
--
2.35.1.windows.2
Reply to: