Closed eldstal closed 1 year ago
Good day.
Good catch. It's clearly wrong. Additionally memcmp should compare using the length of the hash, not the plaint text password.
This example is incomplete, I've had one use case where the entire RPMB was 'sacrificed' to store the password hash. This example is related to that implementation, but since the code needed to setup RPMB is not part of the example, it's close to useless.
I'm going to remove this code block and add some input sanitation in the command parser. I'm leaving this issue open for now.
Thanks/Jonas
Good day!
While performing an audit of an unrelated punchboot fork, I've noticed a potential security flaw in the mainline code. It affects the imx8qxmek board when built with
CONFIG_AUTH_METHOD_PASSWORD=y
.Note: This is untested, as I have no board to run vanilla punchboot on. My apologies for this.
The user performs an
auth
command, which is represented by astruct pb_command_authenticate
:https://github.com/jonasblixt/punchboot/blob/c436ed0f776835fef1c0783ee7b02e62db631071/src/command.c#L299
The
size
field of that struct is used here:https://github.com/jonasblixt/punchboot/blob/c436ed0f776835fef1c0783ee7b02e62db631071/src/command.c#L330
If the auth command is using the password method, that user-controlled(?) size is passed as
length
in a call toboard_command_mode_auth(char *password, size_t length)
:https://github.com/jonasblixt/punchboot/blob/8124fe1a43a07510ecf5ebfb0af671317f99ec2f/src/board/imx8qxmek/board.c#L278
The proper secret is read from storage and the incoming password is hashed. The two are then compared using
memcmp(secret, hash, length)
here:https://github.com/jonasblixt/punchboot/blob/8124fe1a43a07510ecf5ebfb0af671317f99ec2f/src/board/imx8qxmek/board.c#L337
According to POSIX,
memcmp(X, Y, 0)
always returns zero, i.e. authentication succeeds.If the
size
is indeed controlled by the user, this appears to allow for a complete authentication bypass using an auth command withsize=0
.If possible, please confirm this vulnerability. The fix ought to be to set the length of
memcmp()
to match the length of the hash digest rather than the size of the input.Thank you for your time!