Netdex / android-usb-script

An Android app that allows you to script USB gadgets (work-in-progress).
MIT License
37 stars 12 forks source link

Add the possibility to pass a file handle directly to `luausb.create()` #22

Closed C-512L closed 3 months ago

C-512L commented 3 months ago

I want to check a few things on the backing file of a UMS before using it by using io.open() so It would be neat if you could directly pass a file handle instead of a path when creating a USB Mass Storage device. Also, this would avoid code that can be vulnerable to TOCTOU-type attacks. This is how i conceptualize it:

file = io.open("<some path>","r")

local _ = luausb.create({
   type = "storage"
   -- Maybe add as a new key-value to read from? 
   -- if that would be the case a file handle should be preferred over a file path if both are passed
   file = file
 })

I'm not sure how hard would it be to get the file handle in java from LuaJ so I would love some feedback on how possible would be.

Netdex commented 3 months ago

It should be fairly straightforward to marshal the file handle into Java. Unfortunately, I don't think it's possible to use a file handle since the FunctionFS API of the mass storage gadget only accepts a path to a backing file or block device. The handle held by io.open is not exclusive so I'm not sure the TOCTOU condition is avoidable here, though I'm open to being proved wrong.

  The mass storage gadget accepts the following mass storage specific
  module parameters:

  - file=filename[,filename...]

    This parameter lists paths to files or block devices used for
    backing storage for each logical unit.  There may be at most
    FSG_MAX_LUNS (8) LUNs set.  If more files are specified, they will
    be silently ignored.  See also “luns” parameter.

    *BEWARE* that if a file is used as a backing storage, it may not
    be modified by any other process.  This is because the host
    assumes the data does not change without its knowledge.  It may be
    read, but (if the logical unit is writable) due to buffering on
    the host side, the contents are not well defined.

    The size of the logical unit will be rounded down to a full
    logical block.  The logical block size is 2048 bytes for LUNs
    simulating CD-ROM, block size of the device if the backing file is
    a block device, or 512 bytes otherwise.

https://www.kernel.org/doc/Documentation/usb/mass-storage.txt

C-512L commented 3 months ago

It should be fairly straightforward to marshal the file handle into Java. Unfortunately, I don't think it's possible to use a file handle since the FunctionFS API of the mass storage gadget only accepts a path to a backing file or block device. The handle held by io.open is not exclusive so I'm not sure the TOCTOU condition is avoidable here, though I'm open to being proved wrong.

  The mass storage gadget accepts the following mass storage specific
  module parameters:

  - file=filename[,filename...]

    This parameter lists paths to files or block devices used for
    backing storage for each logical unit.  There may be at most
    FSG_MAX_LUNS (8) LUNs set.  If more files are specified, they will
    be silently ignored.  See also “luns” parameter.

    *BEWARE* that if a file is used as a backing storage, it may not
    be modified by any other process.  This is because the host
    assumes the data does not change without its knowledge.  It may be
    read, but (if the logical unit is writable) due to buffering on
    the host side, the contents are not well defined.

    The size of the logical unit will be rounded down to a full
    logical block.  The logical block size is 2048 bytes for LUNs
    simulating CD-ROM, block size of the device if the backing file is
    a block device, or 512 bytes otherwise.

https://www.kernel.org/doc/Documentation/usb/mass-storage.txt

You are right. Another approach could be to keep using paths and make use of file locking to restrict filesystem access to the file beforehand so that only the runtime process can access it. Lua doesn't have file locking as part of its standard library, but maybe this could be added from the Java side so that scripts can create file locks.

Netdex commented 3 months ago

It does not seem like there is a portable way to exclusively lock files across the filesystem. It might be possible to use java.nio.channels.FileLock to implement this API, however this API probably uses advisory locking since mandatory locking is no longer supported since Linux 5.15. UsbGadgetFunctionMassStorage could probably be changed to hold an advisory lock over the duration which the mass storage gadget exists, which would at least could allow preventing Lua file operations from conflicting with the mass storage gadget (e.g., preventing writes from Lua while the mass storage gadget is using the backing file). However, it would not prevent other processes from modifying the backing file.

C-512L commented 3 months ago

It does not seem like there is a portable way to exclusively lock files across the filesystem. It might be possible to use java.nio.channels.FileLock to implement this API, however this API probably uses advisory locking since mandatory locking is no longer supported since Linux 5.15. UsbGadgetFunctionMassStorage could probably be changed to hold an advisory lock over the duration which the mass storage gadget exists, which would at least could allow preventing Lua file operations from conflicting with the mass storage gadget (e.g., preventing writes from Lua while the mass storage gadget is using the backing file). However, it would not prevent other processes from modifying the backing file.

Also after reading the fcntl() man page a little I saw it requires the filesystem to be mounted with specific mount options (which I think falls out of scope for the project). I guess is better to let filesystem permissions handle it (which scripts can already manipulate). In the end, It doesn't improve much when code runs with root privileges, and advisory locking wouldn't be a bad addition, but wouldn't be anything but just a nice-to-have improvement.

I might do some experiments, but I think it's better to close this issue for now.