Two bugs in fs/isofs

Hi,

during regression tests with libisofs i found two bugs in fs/isofs, which i reported to Debian

"fs/isofs/util.c iso_date() will map years >= 2028 to 1970"

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800627

"fs/isofs/rock.c coarsely truncates file names of 254 or 255 bytes length"

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=798300

I got the advise to mail to the addresses i am using now. My apologies for not being used to the submission customs of Linux kernel development. Please give me instructions if i made mistakes which hamper processing.

I have tested my change proposals in a VM with several ISO images as virtual CD-ROM. No negative effects to see.

Bugs in detail (with patches):

===============================================================
"fs/isofs/util.c iso_date() will map years >= 2028 to 1970"
---------------------------------------------------------------

Reproduce by:

# File "/bin/true" is assumed to exist and be readable. # Else use any other readable file instead.

  xorriso -outdev test.iso \
          -blank as_needed \
          -map /bin/true /victim \
          -alter_date m 'Oct 01 22:06:12 2030' /victim --

mount -o loop test.iso /mnt/iso

ls -l /mnt/iso/victim

shows

-rwxr-xr-x 1 root root 27080 Jan 1 1970 /mnt/iso/victim

--------------- Source analysis:

The function iso_date() interprets the unsigned bytes of ISO 9660 7-byte timestamps as 7 chars. By

	if (year < 0) {
		crtime = 0;

it maps years 1900+128 ... 1900+255 to 1970, if type char is signed.

iso_date() sends int parameters where mktime64() expects unsigned int. Further it does not make sure that the parameters are in the ranges expected by mktime64().

Finally, if year would get interpreted as unsigned char, the return type of iso_date() would roll over in year 2038, unless int becomes 64 bit until then.

--------------- Remedy proposal:

* Repair isofs function iso_date():
+ Change return type to time64_t.
+ Interpret bytes from ISO 9660 with appropriate signedness.
+ Map illegal byte values to defaults.
+ Remove test and handling for (year < 0), which is no longer possible.

@@ -103,7 +103,7 @@ static inline unsigned int isonum_733(ch
 	/* Ignore bigendian datum due to broken mastering programs */
 	return get_unaligned_le32(p);
 }
-extern int iso_date(char *, int);
+extern time64_t iso_date(char *, int);
 
 struct inode;		/* To make gcc happy */
 

===============================================================
"fs/isofs/rock.c coarsely truncates file names of 254 or 255 bytes length"
---------------------------------------------------------------

Reproduce by:

# Create Rock Ridge file name of 254 bytes length. # File "/bin/true" is assumed to exist and be readable. # Else use any other readable file instead.

genisoimage -R -o test.iso -graft-points /12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234=/bin/true

mount -o loop test.iso /mnt/iso

ls /mnt/iso

shows

12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456

That's 146 characters only.
If you do the same with xorriso you get other truncation lengths.

--------------- Source analysis:

There is a deliberate limit of < 254 in fs/isofs/rock.c

                       if ((strlen(retname) + rr->len - 5) >= 254) {
                                truncate = 1;
                                break;
                        }

It is not clear to me why there should be such a limit. In the Debian bug report i discuss my findings about clients of the Rock Ridge name reader.
Length up to 255 should be ok.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=798300

The truncation drops the whole second NM entry which contains the rest of the file name bytes. In above example it contained the 108 missing digits.

Truncation nowadays has to take into respect that UTF-8 may consist of multiple bytes and should avoid to leave incomplete byte sequences.
(Does the kernel have a function for this ?)

The truncated names are not necessarily unique within the directory. (There is few chance to check this when the name gets composed from NM entries.)

--------------- Remedy proposal:

I implemented truncation in libisofs this way (from man xorriso):

Path components which are longer than the given
number [64 to 255] will get truncated and have their last 33 bytes overwritten by a colon ':' and the hex representation of the MD5 of the first 4095 bytes of the whole oversized name. Potential
incomplete UTF-8 characters will get their leading bytes
replaced by '_'.

This gives hope for unique truncated names and an opportunity for implementing lookup via the oversized untruncated name. The situation in libisofs gets a bit more complicated by the number being adjustable (for Linux kernels which will be old in future).

If there is interest, i would try to port the truncation and the lookup algorithms from libisofs to Linux. But i guess that i am not the first one who needs to truncate UTF-8. So possibly there are better ways than my userland code.

In my test kernel i only implemented and tested protection of the innocent for now:

* Allow Rock Ridge names of 254 and 255 bytes length.

@@ -267,7 +267,7 @@ repeat:
 					rr->u.NM.flags);
 				break;
 			}
-			if ((strlen(retname) + rr->len - 5) >= 254) {
+			if ((strlen(retname) + rr->len - 5) >= 256) {
 				truncate = 1;
 				break;
 			}

===============================================================

In https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800627
i mentioned a "third bug".

That one turned out to be a cooperation of Linux function isofs_normalize_block_and_offset() and FreeBSD's program makefs, which puts different timestamp equipment into '.' and the directory record which actually holds the directory name.
(I have posted FreeBSD PR 203531 about four flaws of makefs produced
ISO filesystems.)

Have a nice day :)

Thomas

Hi,

thanks for detailed reports. For now I did some research on the case of
file name truncation.

Looking into the code, we should be fine with names upto 1023 characters long (that's the buffer space we have available in that code). However I guess there's no point in supporting more than NAME_MAX (255). So I agree with you here the limit of 254 seems to be off by two. I've tried to dig in history but that code seems to predate even BitKeeper times...

Yeah, that should be reasonably easy to fix.

Well, such truncation function would have to be specific to encoding the fs uses.

Well, true but is it worth the bother? I mean realistically, do people use media with more than 255 characters in a file name or is it mostly a theoretical concern?

Honza

Jan Kara <[email protected]>
SUSE Labs, CR