[marathon.durandal@gmail.com: Re: Unsolicited GNU bc patch]
----- Forwarded message from SDA <marathon.durandal@gmail.com> -----
Date: Thu, 4 Aug 2022 20:37:47 -0400
From: SDA <marathon.durandal@gmail.com>
Subject: Re: Unsolicited GNU bc patch
To: debian-user@lists.debian.org
On Sat, Jul 30, 2022 at 07:29:23AM +0000, Thomas DiModica wrote:
Hi, Thomas. I'd send this to the debian-developer email list rather than
this user list. Not sure if any developers are lurking here ...
> Greetings,
>
> 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.
>
> 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
>
----- End forwarded message -----
Reply to: