[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

vector-of-bool · 2022-08-04T20:08:02Z

Refer to CDRIVER-4448 for details

If bson_iter_visit_all() sets an error offset in the iterator, store that as the error offset for the validation.

Fixes CDRIVER-4448

eramongodb · 2022-08-04T20:12:24Z

src/libbson/src/bson/bson.c

@@ -3584,7 +3584,19 @@ _bson_iter_validate_document (const bson_iter_t *iter,
      state->phase = BSON_VALIDATE_PHASE_LF_REF_KEY;
   }

-   (void) bson_iter_visit_all (&child, &bson_validate_funcs, state);
+   bson_iter_visit_all_v2 (


Should the result of this call be part of the condition below? e.g.

if (!bson_iter_visit_all_v2 ( &child, &bson_validate_funcs, BSON_ITER_VISIT_NOFLAGS, state) && child.err_off > 0 && state->err_offset < 0) {

By careful reading of the source, the return value from this function is nearly meaningless, because it can return true or false both for success and for failure. It's just quite unfortunate that we're trying to cram too much information into a single bool.

eramongodb · 2022-08-04T20:26:16Z

src/libbson/src/bson/bson.c

+                      "Iteration failed on element at offset %d",
+                      (int) (iter->off + child.err_off));


Suggested change

"Iteration failed on element at offset %d",

(int) (iter->off + child.err_off));

"Iteration failed on element at offset %" PRIu32,

iter->off + child.err_off);

eramongodb · 2022-08-04T20:26:50Z

src/libbson/src/bson/bson.c

+
+   if (child.err_off > 0 && state->err_offset < 0) {
+      // Iteration on a direct element of 'child' failed
+      state->err_offset = child.err_off;


There is currently no test coverage of this branch.

Actually, I believe the addition of BSON_ITER_VISIT_NOFLAGS makes this branch unreachable, as child.err_off is only assigned when visit_all fails to validate an element, and we're now disabling all of those checks. Now I'm getting more suspicious. I'll add more tests, just to make sure.

eramongodb · 2022-08-04T20:29:48Z

src/libbson/doc/bson_iter_visit_all_v2.rst

+.. note::
+
+  If a requested element fails validation, the ``bson_iter_visit_all_v2`` call
+  will return and indicate the position of the erring element via ``iter``.


Suggested change

will return and indicate the position of the erring element via ``iter``.

will return and indicate the position of the erring element via ``iter->err_off``.

eramongodb · 2022-08-04T20:39:33Z

src/libbson/src/bson/bson.c

+   bson_iter_visit_all_v2 (
+      &child, &bson_validate_funcs, BSON_ITER_VISIT_NOFLAGS, state);
+
+   if (child.err_off > 0 && state->err_offset < 0) {


Suggested change

if (child.err_off > 0 && state->err_offset < 0) {

if (child.err_off > 0 && state->err_offset <= 0) {

Should it be state->err_offset <= 0 for consistency with the condition used below (line 3604)? Or should the condition below be changed to state->err_offset < 0?

- If bson_iter finds an invalid key string, it will indicate this as a corrupt element. This differs from the rest of its element validation, which will otherwise just halt visitation. - If bson_iter generates an error, we assert that it also informed the validator about this error, rather than trying to recover that error information ourselves.

kevinAlbs · 2022-08-05T15:41:23Z

src/libbson/src/bson/bson-iter.h

+bson_iter_visit_all_v2 (bson_iter_t *iter,
+                        const bson_visitor_t *visitor,
+                        enum bson_iter_visit_flags flags,
+                        void *data);


Consider an alternative API with options:

typedef struct _bson_visitor_opts_t bson_visitor_opts_t; BSON_EXPORT (bson_visitor_opts_t *) bson_visitor_opts_new (bson_iter_visit_flags flags); BSON_EXPORT (void) bson_visitor_opts_destroy (bson_visitor_opts_t *opts); BSON_EXPORT (bool) bson_iter_visit_all_with_opts (bson_iter_t *iter, const bson_visitor_t *visitor, const bson_visitor_opts_t *opts, void *data);

The with_opts name is consistent with other cases where functions required extension. For example: bson_as_json was extended with options as bson_as_json_with_opts.

An options struct allows extension without creating a new function.

kevinAlbs · 2022-08-05T16:41:10Z

src/libbson/src/bson/bson-iter.c

+         if (flags & BSON_ITER_VISIT_VALIDATE_UTF8) {
+            if (!bson_utf8_validate (utf8, utf8_len, allow_nul)) {
+               iter->err_off = iter->off;
+               return true;


Suggested change

return true;

break;

If UTF-8 validation fails, I expect false to be returned (indicating an error) and the visit_corrupt function to be called.

Suggest changing this, and other cases below.

kevinAlbs · 2022-08-05T16:51:26Z

src/libbson/src/bson/bson-iter.c

+bson_iter_visit_all_v2 (bson_iter_t *iter,
+                        const bson_visitor_t *visitor,
+                        enum bson_iter_visit_flags flags,
+                        void *data)
 {


Consider also checking the return values of bson_init_static for the BSON_TYPE_ARRAY and BSON_TYPE_DOCUMENT cases, like the following:

if (!bson_init_static (&b, docbuf, doclen)) { iter->err_off = iter->off; break; } if (VISIT_DOCUMENT (iter, key, &b, data)) { return true; }

Otherwise, an invalid BSON document or array length may be ignored.

kevinAlbs · 2022-08-05T16:54:55Z

src/libbson/src/bson/bson-iter.c

-            iter->err_off = iter->off;
-            return true;
+         if (flags & BSON_ITER_VISIT_VALIDATE_REGEX) {
+            if (!bson_utf8_validate (regex, strlen (regex), allow_nul)) {


Suggested change

if (!bson_utf8_validate (regex, strlen (regex), allow_nul)) {

if (!bson_utf8_validate (regex, strlen (regex), allow_nul) || !bson_utf8_validate (options, strlen (options), allow_nul)) {

kevinAlbs · 2022-08-05T18:31:30Z

src/libbson/src/bson/bson.c

@@ -3584,7 +3584,15 @@ _bson_iter_validate_document (const bson_iter_t *iter,
      state->phase = BSON_VALIDATE_PHASE_LF_REF_KEY;
   }

-   (void) bson_iter_visit_all (&child, &bson_validate_funcs, state);
+   bson_iter_visit_all_v2 (
+      &child, &bson_validate_funcs, BSON_ITER_VISIT_VALIDATE_KEYS, state);


If state.flags has BSON_VALIDATE_UTF8, should BSON_ITER_VISIT_VALIDATE_STRINGS be passed instead?

Same question for BSON_ITER_VISIT_ALLOW_NUL_IN_UTF8.

vector-of-bool added 4 commits August 1, 2022 23:38

Detect iteration failure in bson_validate()

681bd43

If bson_iter_visit_all() sets an error offset in the iterator, store that as the error offset for the validation.

More control over bson_iter_visit validation

fb38261

Document visit_all_v2 API

be101cd

Now we can validate UTF-8 properly

d1ac515

Fixes CDRIVER-4448

vector-of-bool requested review from kevinAlbs and eramongodb August 4, 2022 20:08

eramongodb requested changes Aug 4, 2022

View reviewed changes

kevinAlbs reviewed Aug 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

vector-of-bool commented Aug 4, 2022

eramongodb Aug 4, 2022

vector-of-bool Aug 4, 2022

eramongodb Aug 4, 2022

eramongodb Aug 4, 2022

vector-of-bool Aug 4, 2022

eramongodb Aug 4, 2022

eramongodb Aug 4, 2022

kevinAlbs Aug 5, 2022

kevinAlbs Aug 5, 2022

kevinAlbs Aug 5, 2022

kevinAlbs Aug 5, 2022

kevinAlbs Aug 5, 2022

		"Iteration failed on element at offset %d",
		(int) (iter->off + child.err_off));

	will return and indicate the position of the erring element via ``iter``.
	will return and indicate the position of the erring element via ``iter->err_off``.

	if (child.err_off > 0 && state->err_offset < 0) {
	if (child.err_off > 0 && state->err_offset <= 0) {

	if (!bson_utf8_validate (regex, strlen (regex), allow_nul)) {
	if (!bson_utf8_validate (regex, strlen (regex), allow_nul) \|\| !bson_utf8_validate (options, strlen (options), allow_nul)) {

[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

Are you sure you want to change the base?

[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

Conversation

vector-of-bool commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment