On Thu, Jan 27, 2022 at 07:46:32AM +0100, Theo Buehler wrote:
> On Wed, Jan 26, 2022 at 04:42:04PM +0100, Claudio Jeker wrote:
> > So the RFC is not very clear but in general the idea is that if multiple
> > MFTs are available the newest one (highest manifest number) should be
> > used.
> >
> > In our case there are two possible MFTs available the previously valid on
> > and the now downloaded one. So adjust the parser code so that both files
> > are opened and parsed and the x509 is verified. Checks like the
> > thisUpdate/nextUpdate validity and FileAndHash sequence are postponed.
> > Compare these two mfts and decide which one should be used.
> > Now check everything that was postponed.
> >
> > When checking the hash of files in the MFT check both locations and
> > remember which file was the actual match. It is important that later on
> > the same file is opened.
> >
> > The error checking around MFTs had to be adjusted in some places since it
> > turned out to be too noisy on stale caches.
> >
> > Please test and report unexpected behaviour.
>
> This seems to work fine here. I have read the diff and it looks good,
> but have not reviewed it thoroughly. Do you consider it ready for that?
I have parts I'm not super happy with but have no better idea yet.
Mainly the changes to parse_entity() make that function even more complex
and error prone. proc_parser_mft_check() is the other function where I'm
not sure. So happy for any feedback to improve those bits.
> One thing that stood out in mft_compare():
>
> > +int
> > +mft_compare(const struct mft *a, const struct mft *b)
> > +{
> > + BIGNUM *abn = NULL, *bbn = NULL;
> > + int r;
> > +
> > + if (b == NULL)
> > + return 1;
> > + if (a == NULL)
> > + return 0;
> > +
> > + BN_hex2bn(&abn, a->seqnum);
> > + BN_hex2bn(&bbn, b->seqnum);
>
> These need error checking.
>
> Is it necessary to convert these back into BIGNUMs? Can't we compare
> first the string length and if it's equal do a strcmp?
I think it is possible but I was not sure if it is always correct so I
bailed on that plan and used BN_cmp().
I have to look at how OpenSSL does the BN_bn2hex() conversion. I noticed
that the numbers seen are mostly 2 or 4 bytes long with leding 0.
I agree that this comparision should be easier. Another option would be to
not convert seqnum and keep it as BIGNUM but I'm not a fan of that.
Especially inside struct mft.
> > + r = BN_cmp(abn, bbn);
> > + BN_free(abn);
> > + BN_free(bbn);
> > +
> > + if (r < 0)
> > + return 0;
> > +
> > + /*
> > + * Equal sequence numbers should not happen for different content.
> > + * In this case we prefer the newer MFT.
> > + */
> > + return 1;
> > }
>
--
:wq Claudio
