Re: [PATCH v2] isofs: bound Rock Ridge symlink components to the SL record

[Jan Kara]

On Sun 07-06-26 01:18:27, Bryam Vargas wrote:
> get_symlink_chunk() and the SL handling in
> parse_rock_ridge_inode_internal() walk the variable-length components of
> a Rock Ridge "SL" (symbolic link) record.  Each component is a two-byte
> header (flags, len) followed by len bytes of text, so it occupies
> slp->len + 2 bytes.  Both loops read slp->len and advance to the next
> component, and get_symlink_chunk() additionally does
> memcpy(rpnt, slp->text, slp->len), but neither checks that the component
> lies within the SL record before dereferencing it.
> 
> A crafted SL record whose component declares a len that runs past the
> record (rr->len) therefore triggers an out-of-bounds read of up to 255
> bytes.  When the record sits at the tail of its backing buffer - for
> example a small kmalloc()ed continuation block reached through a CE
> record - the read crosses the allocation; get_symlink_chunk() then
> copies the out-of-bounds bytes into the symlink body returned to user
> space by readlink(), disclosing adjacent kernel memory.
> 
> ISO 9660 images are routinely mounted from untrusted removable media -
> desktop environments auto-mount them (e.g. via udisks2) without
> CAP_SYS_ADMIN - so the record contents are attacker-controlled.
> 
> Reject any component that does not fit in the remaining record bytes
> before using it.  In get_symlink_chunk() return NULL, like the existing
> output-buffer (plimit) checks, so a malformed record makes readlink()
> fail with -EIO rather than silently returning a truncated target; in
> parse_rock_ridge_inode_internal() stop the inode-size walk.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
> Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>

Thanks! I've added the patch to my tree. I've just modified the code in
parse_rock_ridge_inode_internal() to 'goto eio' in case the length is
wrong. We generally do this in that function when we find incorrect on-disk
structure and there's no good reason why we shouldn't refuse broken isofs
images early.

								Honza

> ---
> v2: in get_symlink_chunk() return NULL (-> readlink() -EIO) instead of
>     break on an over-long component, per Michael Bommarito's review.
>     break would let rock_ridge_symlink_read_folio() treat the truncated
>     rpnt as success and hand a silently-truncated symlink to userspace;
>     NULL matches the function's existing plimit-overflow handling and
>     fails closed.  The size walk in parse_rock_ridge_inode_internal()
>     keeps break (it only bounds i_size; the read path now errors).
> 
> Reproducer (crafted ISO-9660 image with Rock Ridge):
> 
>   # a symlink whose SL component length byte is enlarged so the
>   # component overruns its SL record
>   ln -s "$(python3 -c 'print("A"*250)')" /tmp/iso/l
>   genisoimage -R -o rr.iso /tmp/iso
>   # repoint the symlink's CE record to a tight continuation block
>   # (cont_size = 7) holding one SL record:
>   #   53 4c 07 01 00 00 ff   "SL", len 7, ver 1, comp flags 0, comp len 0xff
>   # so rock_continue() does kmalloc(7) and the component text begins at
>   # the end of that allocation.
>   mount -o loop,ro rr.iso /mnt
>   readlink /mnt/l
> 
> Without the patch, get_symlink_chunk() memcpy()s slp->len (0xff) bytes
> starting one byte into the 7-byte allocation, so readlink() returns the
> symlink target followed by adjacent in-kernel bytes.  With the patch the
> over-long component is rejected (slp->len + 2 > slen) and readlink()
> fails with -EIO; a well-formed Rock Ridge image is unaffected.
> 
>  fs/isofs/rock.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
> index 1232fab59a4e..0fe781381e66 100644
> --- a/fs/isofs/rock.c
> +++ b/fs/isofs/rock.c
> @@ -466,6 +466,9 @@ parse_rock_ridge_inode_internal(struct iso_directory_record *de,
>  				inode->i_size = symlink_len;
>  				while (slen > 1) {
>  					rootflag = 0;
> +					/* keep the component within the SL record */
> +					if (slp->len + 2 > slen)
> +						break;
>  					switch (slp->flags & ~1) {
>  					case 0:
>  						inode->i_size +=
> @@ -621,6 +624,14 @@ static char *get_symlink_chunk(char *rpnt, struct rock_ridge *rr, char *plimit)
>  	slp = &rr->u.SL.link;
>  	while (slen > 1) {
>  		rootflag = 0;
> +		/*
> +		 * A component is slp->len + 2 bytes (a two-byte header plus
> +		 * len bytes of text).  If it does not fit in the bytes left in
> +		 * the SL record the record is malformed: fail like the plimit
> +		 * checks below so readlink() returns -EIO, not a truncated path.
> +		 */
> +		if (slp->len + 2 > slen)
> +			return NULL;
>  		switch (slp->flags & ~1) {
>  		case 0:
>  			if (slp->len > plimit - rpnt)
> --
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR