Re: [PATCH] udf: Fix regression which is preventing symlink access

[Glenn Washburn <development@efficientek.com>]

On Tue, 14 Sep 2021 16:27:55 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Sep 10, 2021 at 04:03:23PM +0000, Glenn Washburn wrote:
> > This code was broken by commit 3f05d693 ("malloc: Use overflow
> > checking primitives where we do complex allocations"), which added
> > overflow checking in many areas. The problem here is that the
> > changes update the local variable sz, which was already in use and
> > which was not updated before the change. So the code using sz was
> > getting a different value of than it would have previously for the
> > same UDF image. This causes the logic getting the destination of
> > the symlink to not realize that its gotten the full destination,
> > but keeps trying to read past the end of the destination. The bytes
> > after the end are generally NULL padding bytes, but that's not a
> > valid component type (ECMA-167 14.16.1.1). So grub_udf_read_symlink
> > branches to error logic, returning NULL, instead of the symlink
> > destination path.
> >
> > The result of this bug is that the UDF filesystem tests were
> > failing in the symlink test with the grub-fstest error message:
> >
> >     grub-fstest: error: cannot open `(loop0)/sym': invalid symlink.
> >
> > This change stores the result of doubling sz in another local
> > variable s, so as not to modify sz. Also remove unnecessary
> > grub_add, which increased the output by 1 to account for a NULL
> > byte. This isn't needed because an output buffer of size twice sz
> > is already guaranteed to be more than enough to contain the path
> > components converted to UTF-8. The worst case upper- bound for the
> > needed output buffer size is (sz-4)*1.5, where 4 is the size
> 
> I think 4 comes from ECMA-167 spec. Could you add a reference to it
> here? The number of paragraph would be perfect...

Its 14.16.1 basically in the same place as the reference earlier, which
is why I didn't include it. But, yes, I can include it.

> > of a path component header and 1.5 is the maximum growth in bytes
> > when converting from 2-byte unicode code-points to UTF-8 (from 2
> > bytes to 3).
> 
> Could you explain how did you come up with the 1.5 value? It would be
> nice if you refer to a spec or something like that.

There is no spec that I know of (but would be happy to know of one). Its
something I've deduced based on my understanding of Unicode, UTF-8, and
UTF-16. All unicode code points less than or equal to 2 bytes (code
points <0x10000) can be represented in UTF-8 by a maximum of 3 bytes
[1]. Longer UTF-16 encodings don't matter because those will be 4 bytes
or longer. The maximum number of bytes for a UTF-8 encoding of a unicode
code point is 4 bytes. So the longer UTF-16 encodings can only be equal
to or longer than the UTF-8 encoding, thus the UTF-16 -> UTF-8 would be
shrinking or maintaining the length of the original byte string. Since
the worst case growth is 2 bytes to 3, that's 1.5 times the original
string size. QED.

Do you want all that in there? I could just remove that part about the
1.5 too.

Here's an SO question that addresses this. Yes, unofficial, but I think
adds some weight as to the correctness of the logic above.

https://stackoverflow.com/questions/55056322/maximum-utf-8-string-size-given-utf-16-size

Glenn

[1] https://en.wikipedia.org/wiki/UTF-8#Encoding

> 
> Daniel
> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/fs/udf.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> > index 2ac5c1d00..197e60d12 100644
> > --- a/grub-core/fs/udf.c
> > +++ b/grub-core/fs/udf.c
> > @@ -1022,7 +1022,7 @@ grub_udf_iterate_dir (grub_fshelp_node_t dir,
> >  static char *
> >  grub_udf_read_symlink (grub_fshelp_node_t node)
> >  {
> > -  grub_size_t sz = U64 (node->block.fe.file_size);
> > +  grub_size_t s, sz = U64 (node->block.fe.file_size);
> >    grub_uint8_t *raw;
> >    const grub_uint8_t *ptr;
> >    char *out = NULL, *optr;
> > @@ -1035,11 +1035,19 @@ grub_udf_read_symlink (grub_fshelp_node_t
> > node) if (grub_udf_read_file (node, NULL, NULL, 0, sz, (char *)
> > raw) < 0) goto fail_1;
> >
> > -  if (grub_mul (sz, 2, &sz) ||
> > -      grub_add (sz, 1, &sz))
> > +  /*
> > +   * Local sz is the size of the symlink file data, which contains
> > a sequence
> > +   * of path components (ECMA-167 14.16.1) representing the link
> > destination.
> > +   * This size is an upper-bound on the number of bytes of a
> > contained and
> > +   * potentially compressed 2-byte character string. Allocate 2*sz
> > for the
> > +   * output buffer contained the string converted to UTF-8 because
> > the
> > +   * resulting string can not be more than double the size (2-byte
> > unicode
> > +   * points will be converted to a maximum of 3 bytes in UTF-8).
> > +   */
> > +  if (grub_mul (sz, 2, &s))
> >      goto fail_0;
> >
> > -  out = grub_malloc (sz);
> > +  out = grub_malloc (s);
> >    if (!out)
> >      {
> >   fail_0:
> > @@ -1051,7 +1059,6 @@ grub_udf_read_symlink (grub_fshelp_node_t
> > node)
> >
> >    for (ptr = raw; ptr < raw + sz; )
> >      {
> > -      grub_size_t s;
> >        if ((grub_size_t) (ptr - raw + 4) > sz)
> >  	goto fail_1;
> >        if (!(ptr[2] == 0 && ptr[3] == 0))
> > --
> > 2.32.0