Skip to content

Commit

Permalink
Attempted bugfix for potential session reentrancy bug (#273)
Browse files Browse the repository at this point in the history
Context: iqlusioninc/tmkms#37

To summarize, the `yubihsm::Client` type has internal locking for a
`Session` with the YubiHSM. There is actually very little lock-related
code associated with this scheme, but there appears to be a subtle
reentrancy bug in pretty much the only function where it could possibly
exist: the `yubihsm::Client::send_command` method.

Based on discussions on iqlusioninc/tmkms#37 the bug definitely seemed
to be tied to rekeying the session after a preset number of messages had
been sent. It appears there was a reentrancy bug where an RAII guard was
still holding the `Mutex` for the session state, which prevented a
subsequent attempt to resent the message from acquiring it.

The solution is to explicitly drop the stale session guard prior to
trying to establish a new session.
  • Loading branch information
tony-iqlusion authored Nov 30, 2021
1 parent e61f099 commit 974b19b
Showing 1 changed file with 11 additions and 9 deletions.
20 changes: 11 additions & 9 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,21 +165,23 @@ impl Client {

/// Encrypt a command, send it to the HSM, then read and decrypt the response.
fn send_command<T: Command>(&self, command: T) -> Result<T::ResponseType, Error> {
match self.session()?.send_command(&command) {
let mut session = self.session()?;

match session.send_command(&command) {
Ok(response) => Ok(response),
Err(e) => {
Err(err) if *err.kind() == session::ErrorKind::CommandLimitExceeded => {
// If we encounter this, we've exceeded the maximum number of
// messages allowed under the data volume limits and need to
// rekey the connection by creating a new session.
//
// Attempt to inititiate a new session and retry the command.

// Drop the stale `session::Guard` to release the session mutex.
drop(session);

// Attempt to initiate a new session and retry the command.
// (the original command was never sent in this case)
if *e.kind() == session::ErrorKind::CommandLimitExceeded {
Ok(self.session()?.send_command(&command)?)
} else {
Err(e.into())
}
Ok(self.session()?.send_command(&command)?)
}
Err(err) => Err(err.into()),
}
}

Expand Down

0 comments on commit 974b19b

Please sign in to comment.