Fix unsafe dereferences of fr_sbuff_current

Add significantly simpler error handling logic

diff --git a/src/bin/radsniff.c b/src/bin/radsniff.c
index 248afffbd11226d6245df9e135e225e1cdf8efa8..201c9ef5e5871ec18f9414ee672b64dc6fbe7075 100644 (file)
--- a/src/bin/radsniff.c
+++ b/src/bin/radsniff.c
@@ -375,7 +375,7 @@ static void rs_packet_print_csv(uint64_t count, rs_status_t status, fr_pcap_t *h
 
                memset(fr_sbuff_current(&sbuff), ',', conf->list_da_num);
                fr_sbuff_advance(&sbuff, conf->list_da_num);
-               *fr_sbuff_current(&sbuff) = '\0';
+               fr_sbuff_terminate(&sbuff);
        }
 
        fprintf(stdout , "%s\n", buffer);

diff --git a/src/bin/unit_test_attribute.c b/src/bin/unit_test_attribute.c
index c210e52396c6b7c4e707e5818a7c998a135811c7..c578fd2f23d1ebfa01195e3bf348e5392d293e08 100644 (file)
--- a/src/bin/unit_test_attribute.c
+++ b/src/bin/unit_test_attribute.c
@@ -3549,7 +3549,7 @@ static int line_ranges_parse(TALLOC_CTX *ctx, fr_dlist_head_t *out, fr_sbuff_t *
                        goto error;
                }
 
-               switch (*fr_sbuff_current(in)) {
+               fr_sbuff_switch(in, '\0') {
                /*
                 *      More ranges...
                 */
@@ -3847,7 +3847,7 @@ int main(int argc, char *argv[])
                        while (fr_sbuff_extend(&in)) {
                                fr_sbuff_adv_until(&in, SIZE_MAX, &dir_sep, '\0');
 
-                               switch (*fr_sbuff_current(&in)) {
+                               fr_sbuff_switch(&in, '\0') {
                                case '/':
                                        fr_sbuff_set(&dir_end, &in);
                                        fr_sbuff_advance(&in, 1);

diff --git a/src/lib/ldap/filter.c b/src/lib/ldap/filter.c
index 96ff71fec10271de9ff2464fc8f204243c9d6a4c..8bf4eeb409ab5536409c45d09974712293a045ca 100644 (file)
--- a/src/lib/ldap/filter.c
+++ b/src/lib/ldap/filter.c
@@ -60,7 +60,7 @@ static fr_slen_t ldap_filter_parse_logic(ldap_filter_t *node, fr_sbuff_t *sbuff,
        fr_slen_t       ret = 0;
        fr_slen_t       parsed = 0;
 
-       switch(*fr_sbuff_current(sbuff)) {
+       fr_sbuff_switch(sbuff, '\0') {
        case '&':
                node->logic_op = LDAP_FILTER_LOGIC_AND;
                node->orig = talloc_typed_strdup(node, "&");

diff --git a/src/lib/server/users_file.c b/src/lib/server/users_file.c
index fc957c8dc89e30a209fb3350a0449ae9e1f4a45d..86d2420a76bb1073e694843855f07f125695c55a 100644 (file)
--- a/src/lib/server/users_file.c
+++ b/src/lib/server/users_file.c
@@ -183,7 +183,7 @@ static int users_include(TALLOC_CTX *ctx, fr_dict_t const *dict, fr_sbuff_t *sbu
         *      Otherwise the $INCLUDE name is an absolute path, use
         *      it as -is.
         */
-       c = *fr_sbuff_current(&name);
+       c = fr_sbuff_char(&name, '\0');
        if (c != '/') {
                p = strrchr(file, '/');
 

diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index e8e99ddad296090f4160c3fa69a7b79c591cc9b7..9bef82b66e904bcc6bbc51cf6eba0e6a55cb9a4d 100644 (file)
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -1824,7 +1824,7 @@ static const int precedence[T_TOKEN_LAST] = {
 
 #define fr_sbuff_skip_whitespace(_x) \
        do { \
-               while (isspace((int) *fr_sbuff_current(_x))) fr_sbuff_advance(_x, 1); \
+               while (isspace((int) fr_sbuff_char(_x, '\0'))) fr_sbuff_advance(_x, 1); \
        } while (0)
 
 static ssize_t tokenize_expression(xlat_exp_head_t *head, xlat_exp_t **out, fr_sbuff_t *in,

diff --git a/src/lib/unlang/xlat_tokenize.c b/src/lib/unlang/xlat_tokenize.c
index b50fb005da9c1616ec63e257c04390cdfa6ee1ff..60baa5254249970edae945b1971ab2328b89a0d7 100644 (file)
--- a/src/lib/unlang/xlat_tokenize.c
+++ b/src/lib/unlang/xlat_tokenize.c
@@ -817,13 +817,13 @@ int xlat_tokenize_expansion(xlat_exp_head_t *head, fr_sbuff_t *in,
         *
         *      e.g. '%{myfirstxlat'
         */
-       if (!fr_sbuff_remaining(in)) {
+       if (!fr_sbuff_extend(in)) {
                fr_strerror_const("Missing closing brace");
                fr_sbuff_marker_release(&s_m);
                return -1;
        }
 
-       hint = *fr_sbuff_current(in);
+       hint = fr_sbuff_char(in, '\0');
 
        XLAT_DEBUG("EXPANSION HINT TOKEN '%c'", hint);
        if (len == 0) {

diff --git a/src/lib/util/base32.c b/src/lib/util/base32.c
index 8da1f32d8207825289fb6f9a856598ff15a5b8f5..8a59aa18deccc20e70dc5bebe8f0a4bb5982e5f5 100644 (file)
--- a/src/lib/util/base32.c
+++ b/src/lib/util/base32.c
@@ -359,7 +359,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
        /*
         *      Find the first non-base32 char
         */
-       while (fr_sbuff_extend(&our_in) && fr_is_base32_nstd(*fr_sbuff_current(&our_in), alphabet)) {
+       while (fr_sbuff_extend(&our_in) && fr_is_base32_nstd(fr_sbuff_char(&our_in, '\0'), alphabet)) {
                fr_sbuff_advance(&our_in, 1);
        }
 
@@ -439,7 +439,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
                        }
                        if (!fr_sbuff_next_if_char(&our_in, '=')) {
                                fr_strerror_printf("Found non-padding char '%c' at end of base32 string",
-                                                  *fr_sbuff_current(&our_in));
+                                                  fr_sbuff_char(&our_in, '\0'));
 
                                if (err) *err = FR_SBUFF_PARSE_ERROR_FORMAT;
 
@@ -450,7 +450,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 
        if (no_trailing && fr_sbuff_extend(&our_in)) {
                fr_strerror_printf("Found trailing garbage '%c' at end of base32 string",
-                                  *fr_sbuff_current(&our_in));
+                                  fr_sbuff_char(&our_in, '\0'));
 
                if (err) *err = FR_SBUFF_PARSE_ERROR_TRAILING;
 

diff --git a/src/lib/util/base64.c b/src/lib/util/base64.c
index 51c1146a0abb7d8af8371358782d2b6d612c888c..15f6895849170247ca97070a8f38c5303e0f688f 100644 (file)
--- a/src/lib/util/base64.c
+++ b/src/lib/util/base64.c
@@ -438,7 +438,7 @@ ssize_t     fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
        /*
         *      Find the first non-base64 char
         */
-       while (fr_sbuff_extend(&our_in) && fr_is_base64_nstd(*fr_sbuff_current(&our_in), alphabet)) {
+       while (fr_sbuff_extend(&our_in) && fr_is_base64_nstd(fr_sbuff_char(&our_in, '\0'), alphabet)) {
                fr_sbuff_advance(&our_in, 1);
        }
 
@@ -487,7 +487,7 @@ ssize_t     fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
                        }
                        if (!fr_sbuff_next_if_char(&our_in, '=')) {
                                fr_strerror_printf("Found non-padding char '%c' at end of base64 string",
-                                                  *fr_sbuff_current(&our_in));
+                                                  fr_sbuff_char(&our_in, '\0'));
                                goto bad_format;
                        }
                }
@@ -495,7 +495,7 @@ ssize_t     fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 
        if (no_trailing && fr_sbuff_extend(&our_in)) {
                fr_strerror_printf("Found trailing garbage '%c' at end of base64 string",
-                                  *fr_sbuff_current(&our_in));
+                                  fr_sbuff_char(&our_in, '\0'));
 
                if (err) *err = FR_SBUFF_PARSE_ERROR_TRAILING;
 

diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c
index dde5faa40c06fbc6022fab126a7b7527c305d828..f98559cc9303a3f1e9d38cf738e8c0c09d7a2764 100644 (file)
--- a/src/lib/util/dict_util.c
+++ b/src/lib/util/dict_util.c
@@ -2970,7 +2970,7 @@ ssize_t   fr_dict_enum_by_name_substr(fr_dict_enum_value_t **out, fr_dict_attr_t c
                int len = (p - name) + 1;
                fr_dict_enum_value_t *enumv;
 
-               *p = *fr_sbuff_current(&our_in);
+               *p = fr_sbuff_char(&our_in, '\0');
                if (!fr_dict_enum_allowed_chars[*p]) {
                        break;
                }

diff --git a/src/lib/util/sbuff.c b/src/lib/util/sbuff.c
index 76e389069de5ad87529648c87b6024061e285bdb..be38511cb1d45659ff847347a3ab9392e7bb567e 100644 (file)
--- a/src/lib/util/sbuff.c
+++ b/src/lib/util/sbuff.c
@@ -1068,7 +1068,7 @@ fr_slen_t fr_sbuff_out_bool(bool *out, fr_sbuff_t *in)
        };
 
        if (fr_sbuff_is_in_charset(&our_in, bool_prefix)) {
-               switch (tolower(*fr_sbuff_current(&our_in))) {
+               switch (tolower(fr_sbuff_char(&our_in, '\0'))) {
                default:
                        break;
 

diff --git a/src/lib/util/sbuff.h b/src/lib/util/sbuff.h
index 79aa855899da2135cecc4e85cbd4150ce4494c17..fc2db1a06d7365b84138e74325ca7790d797fc35 100644 (file)
--- a/src/lib/util/sbuff.h
+++ b/src/lib/util/sbuff.h
@@ -88,6 +88,8 @@ struct fr_sbuff_s {
                char *p;                                        //!< Mutable position pointer.
        };
 
+       char const *err;                                //!< Where the last error occurred.
+
        uint8_t                 is_const:1;             //!< Can't be modified.
        uint8_t                 adv_parent:1;           //!< If true, advance the parent.
 
@@ -895,14 +897,35 @@ static inline fr_sbuff_t *fr_sbuff_init_talloc(TALLOC_CTX *ctx,
        ((size_t)(fr_sbuff_start(_sbuff_or_marker) > fr_sbuff_current(_sbuff_or_marker) ? \
                0 : (fr_sbuff_current(_sbuff_or_marker) - fr_sbuff_start(_sbuff_or_marker))))
 
+
+/** Sets an error marker in the parent
+ *
+ * If an error already exists at this level it will be used instead of the provided error.
+ *
+ * @param[in] sbuff    who's parent we'll set the error marker in.
+ * @param[in] err      marker to set.
+ * @return <0 the negative offset of the error.
+ */
+static inline fr_slen_t _fr_sbuff_error(fr_sbuff_t *sbuff, char const *err)
+{
+       fr_sbuff_t *parent = sbuff->parent;
+
+       if (sbuff->err) err = sbuff->err;
+       if (parent) parent->err = err;
+
+       return -((err - fr_sbuff_start(sbuff)) + 1);
+}
+
 /** Return the current position as an error marker
+ *
+ * @param[in] _sbuff_or_marker         Error marker will be set from the current position of this sbuff.
  *
  * +1 is added to the position to disambiguate with 0 meaning "parsed no data".
  *
  * An error at offset 0 will be returned as -1.
  */
 #define fr_sbuff_error(_sbuff_or_marker) \
-       (-(fr_sbuff_used(_sbuff_or_marker) + 1))
+       _fr_sbuff_error(fr_sbuff_ptr(_sbuff_or_marker), fr_sbuff_current(_sbuff_or_marker));
 
 /** Like fr_sbuff_used, but adjusts for the value returned for the amount shifted
  *
@@ -1045,6 +1068,8 @@ do { \
 static inline void _fr_sbuff_set_recurse(fr_sbuff_t *sbuff, char const *p)
 {
        sbuff->p_i = p;
+       sbuff->err = NULL;      /* Modifying the position of the sbuff clears the error */
+
        if (sbuff->adv_parent && sbuff->parent) _fr_sbuff_set_recurse(sbuff->parent, p);
 }
 
@@ -1056,6 +1081,7 @@ static inline ssize_t _fr_sbuff_marker_set(fr_sbuff_marker_t *m, char const *p)
        if (unlikely(p > sbuff->end)) return -(p - sbuff->end);
        if (unlikely(p < sbuff->start)) return 0;
 
+       sbuff->err = NULL;      /* Modifying the position of any markers clears the error, unsure if this is correct? */
        m->p_i = p;
 
        return p - current;

diff --git a/src/lib/util/size.c b/src/lib/util/size.c
index 7e6a5b62a7ec39b5309fd9afe3ff3a3e83180bb9..f8fd3ee2f5f9fe7fd64731cc78feff43d02f3cb7 100644 (file)
--- a/src/lib/util/size.c
+++ b/src/lib/util/size.c
@@ -68,7 +68,7 @@ fr_slen_t fr_size_from_str(size_t *out, fr_sbuff_t *in)
        if (fr_sbuff_out(NULL, &size, &our_in) < 0) return fr_sbuff_error(&our_in);
        if (!fr_sbuff_extend(&our_in)) goto done;
 
-       c = tolower(*fr_sbuff_current(&our_in));
+       c = tolower(fr_sbuff_char(&our_in, '\0'));
 
        /*
         *      Special cases first...

diff --git a/src/lib/util/uri.c b/src/lib/util/uri.c
index 3cd3ea08d1d84b4ff65358611e756ff58dfd7969..7ed1df776e5362f5fc1a75b9a4084ef24688b9cd 100644 (file)
--- a/src/lib/util/uri.c
+++ b/src/lib/util/uri.c
@@ -43,7 +43,6 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
        fr_value_box_t          *uri_vb = NULL;
        fr_uri_part_t const     *uri_part;
        fr_sbuff_t              sbuff;
-       char const              *p;
 
        uri_part = uri_parts;
 
@@ -92,12 +91,11 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
 
                do {
                        fr_sbuff_adv_until(&sbuff, SIZE_MAX, uri_part->terminals, '\0');
-                       p = fr_sbuff_current(&sbuff);
 
                        /*
                         *      We've not found a terminal in the current box
                         */
-                       if (uri_part->part_adv[(uint8_t)*p] == 0) continue;
+                       if (uri_part->part_adv[fr_sbuff_char(&sbuff, '\0')] == 0) continue;
 
                        /*
                         *      This terminator has trailing characters to skip
@@ -107,7 +105,7 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
                        /*
                         *      Move to the next part
                         */
-                       uri_part += uri_part->part_adv[(uint8_t)*p];
+                       uri_part += uri_part->part_adv[fr_sbuff_char(&sbuff, '\0')];
                        if (!uri_part->terminals) break;
                } while (fr_sbuff_advance(&sbuff, 1) > 0);
        }