Allow members of plugdev group to execute rpiboot without root #27
Conversation
Give members of plugdev group access to the usb device by udev rules.
|
For reference, I suggested plugdev because this is the group you need to be in to use avrdude with USB programmers under Debian. It seems appropriate, but you could invent your own group for this if you wanted. |
|
It's also used for other removable stuff like phones, cameras and storage. |
| @@ -332,6 +332,12 @@ FILE * check_file(char * dir, char *fname) | |||
| FILE * fp = NULL; | |||
| char path[256]; | |||
|
|
|||
| // Prevent USB device from requesting files in parent directories | |||
| if(strstr(fname, "..")) | |||
| { | |||
There was a problem hiding this comment.
What's the reason for this? Can you just add an error message to explain why this is disabled?
There was a problem hiding this comment.
Patchset aims to make it safer to allow unprivileged users in multi-user setups run rpiboot, without effectively handing them the keys to the entire system.
rpiboot essentially is file server software handing out whatever file any attached USB device claiming to be a Raspberry Pi asks for.
If I enter rpiboot /usr/share/rpiboot/msd I have the reasonable expectation that the file server code only shares my /usr/share/rpiboot/msd folder with the connected device.
However currently nothing prevents a device to ask for a file name like ../../../../etc/shadow, which will get translated to /usr/share/rpiboot/msd/../../../../etc/shadow and serve /etc/shadow
Simply rejecting any request with a .. in it is the simplest form of protection against directory traversal attacks like this.
Could also turn the path provided into an absolute path first and verify if that is within the path allowed however that requires platform specific code.
There was a problem hiding this comment.
Also note that this isn't completely mitigated by dropping root. It could also try stuff like ../.ssh/id.rsa to steal your ssh key.
+1 for adding an error message. Also since you removed the sudo check error message, I think a new error message should be added at whatever point the transfer fails if the user is not in plugdev.
There was a problem hiding this comment.
Correction: it does allow enumeration without privileges.
Added a patch to print a message.
There was a problem hiding this comment.
It would be good to see the path traversal patch being merged soon, testing against a stock rpiboot as expected it happily sends "../../../../etc/shadow".
Found serial number 1
Second stage boot server
Received message GetFileSize: ../../../../etc/shadow
File size = 885 bytes
Received message ReadFile: ../../../../etc/shadow
File read: ../../../../etc/shadow
And with the "Disallow requests for files outside directory" patch it's denied.
Second stage boot server
Received message GetFileSize: ../../../../etc/shadow
Cannot open file ../../../../etc/shadow
Received message ReadFile: ../../../../etc/shadow
No file ../../../../etc/shadow found
* Error out if permission was denied while opening device. * Show error if request for file containing .. was denied.
|
@ghollingworth Is there anything that needs to be done before this can be merged? Would be nice to get it in, both because of the vulnerability and to make it easier/more secure without |
|
@maxnet You happy with this in it's current state? |
|
Am happy with it. Be aware it has only been tested under Linux though, and not under MacOS and such. |
|
I think the preferred way would be to use the |
|
Why wasn't this merged for so long? #44 is absolutely right. |
…errypi#27) * Do not require root privileges Give members of plugdev group access to the usb device by udev rules. * Disallow requests for files outside directory * Be more verbose about errors * Error out if permission was denied while opening device. * Show error if request for file containing .. was denied.
It would be nice if it was possible to execute rpiboot without root privileges.
To be able to talk to the device it's only necessary that the user has write permissions on the right /dev/bus/usb device node.
Give members of plugdev group access by udev rules.
As suggested by @ali1234 in raspberrypi/usbbootgui#3