On 01/10/2007, Jean-Pierre André wrote: > Hi Yuval, > > Thanks for your review and all your valuable comments. > > I take all of them into account, except this one, for which I > am at a loss for what to do : > > > In build_permissions() you use: > > adminowns = !memcmp(usid,adminsid(),usidsz) || > > !memcmp(gsid,adminsid(),gsidsz); > > > > You assume that usidsz >= 16. It is unlikely to crash if not, but it > > is still an assumption you should not make. (think fuzzing) > > > > Where is the assumption ? I have never heard of a limit > in memcmp(). On some architectures, when fetching a byte, > you actually fetch a few unneeded bytes possibly beyond > end of memory allocated to process, but that should be > taken into account by the linker (or malloc()).
This is theoretical and unlikely to be exploitable in practice (on the worst case, it will crash, and _not_ run external code). It is easy to avoid by checking that usidsz == sizeof(ADMIN_SID) prior to the mentioned line. It will speed up some checks too. On a re-read, it seems like I wrongly reversed the operator. You assume that usidsz <= 16. If the on-disk SID reports (for example) 255 sub-authorities, you will compare a >1KB byte block to 16 bytes. memcmp will read after the value returned by adminsid(), which may be unallocated. It may be theoretically possible to craft a SID that will look like the memory after adminsid() because adminsid() should be placed in .rodata, so memcpy() will continue comparing until it overflows the memory page. > Another point, about endianness : I have redefined macroes > to check endianness at compile-time, but I had to redefine > many fields in layout.h because most disk images are > defined as u16, u32 etc. instead of le16, le32, etc. > > Has this been done on purpose ? layout.h is older than sparse. I think the ntfs-3g split was done before Yura ran sparse on libntfs (the non-3g one). > I think the option of enabling compile-time checks is > useful. I have found several errors in my code, but also > two errors outside my code : > > in ntfs_ie_get_next() from index.c there is a le16_to_cpu() > which should be le32_to_cpu() and is wrong on BE boxes. > > in ACCESS_ALLOWED_ACE from layout.h, the field > mask is defined as an enum whose size depends both on > the architecture and the compiler, whereas it should be > defined as a le32. I have not checked the code, but if you are right, I'm sure Szaka will accept a patch. > > Regards > > Jean-Pierre -- Yuval Fledel ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ ntfs-3g-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
