Bug 1663091 - Remove unnecessary assertions in the streaming ASN.1 decoder r=kjacobs

The streaming ASN.1 decoder had assertions that, on debug builds, blocked
embedding indefinite-length fields inside of definite-length fields/contexts,
however that behavior does work correctly, and is valid ASN.1: it tends to
happen when wrapping a signature around existing ASN.1-encoded data, if that
already-encoded data had an indefinite length.

Really these two assertion were just overzealous. The conditional after the
asserts handle the case well, and memory sanitizers have not found issue here
either.

Differential Revision: https://phabricator.services.mozilla.com/D93135

1 files changed, 9 insertions, 2 deletions

diff --git a/lib/util/secasn1d.c b/lib/util/secasn1d.c
index ed237ed72..bb1887f7c 100644
--- a/lib/util/secasn1d.c
+++ b/lib/util/secasn1d.c
@@ -1982,8 +1982,15 @@ sec_asn1d_next_in_group(sec_asn1d_state *state)
          * compensating for "offset", as is done a little farther below
          * in the more normal case.
          */
-        PORT_Assert(state->indefinite);
-        PORT_Assert(state->pending == 0);
+        /*
+         * XXX We used to assert our overall state was that we were decoding
+         * an indefinite-length object here (state->indefinite == TRUE and no
+         * pending bytes in the decoder), but those assertions aren't correct
+         * as it's legitimate to wrap indefinite sequences inside definite ones
+         * and this code handles that case. Additionally, when compiled in
+         * release mode these assertions aren't checked anyway, yet function
+         * safely.
+         */
         if (child->dest && !state->subitems_head) {
             sec_asn1d_add_to_subitems(state, child->dest, 0, PR_FALSE);
             child->dest = NULL;