Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for failure of OpenSSL RAND_[pseudo_]bytes #163

Merged
merged 2 commits into from Oct 26, 2020

Conversation

deastoe
Copy link
Contributor

@deastoe deastoe commented Oct 23, 2020

Discovered by @gollub.

magic.c: check for failure of RAND_[pseudo_]bytes

When magic() is implemented via libcrypto's RAND_bytes or
RAND_pseudo_bytes we should check for a failure and abort to
ensure we don't use a predictable session_id.

This prevents (further) weakening* of the TACACS+ protocol
"encryption" since session_id is an input to the algorithm.

*by modern standards TACACS+ is deemed "obfuscated" - RFC 8907.

pam_tacplus.c: Fallback to using PID as task ID

If there is a failure obtaining a random task ID for the session
accounting request then fallback to using the PID, as this is unique
for the lifetime of the PAM application and therefore session.

When magic() is implemented via libcrypto's RAND_bytes or
RAND_pseudo_bytes we should check for a failure and abort to
ensure we don't use a predictable session_id.

This prevents (further) weakening* of the TACACS+ protocol
"encryption" since session_id is an input to the algorithm.

*by modern standards TACACS+ is deemed "obfuscated" - RFC 8907.
If there is a failure obtaining a random task ID for the session
accounting request then fallback to using the PID, as this is unique
for the lifetime of the PAM application and therefore session.
@gollub
Copy link
Collaborator

gollub commented Oct 23, 2020

LGTM

@deastoe
Copy link
Contributor Author

deastoe commented Oct 23, 2020

I have requested allocation of a CVE ID from MITRE.

@kravietz kravietz merged commit 468cc9d into kravietz:master Oct 26, 2020
1 check passed
@kravietz
Copy link
Owner

Thanks, when we have CVE we can bump version and publish a new release.

@carnil
Copy link

carnil commented Oct 27, 2020

CVE-2020-27743 was assigned for this issue.

@kravietz
Copy link
Owner

Can you review/edit GHSA-rp3p-jm35-jv76

@deastoe
Copy link
Contributor Author

deastoe commented Nov 2, 2020

Thanks all for progressing this while I was AFK.

@carnil
Copy link

carnil commented Nov 4, 2020

@kravietz, @deastoe looking the commit history of the project it looks that the issue only is introduced after 6fac250 in v1.5.0-beta.1. If this is correct maybe in GHSA-rp3p-jm35-jv76 that could be added to the 'affected versions' constraints.

@gollub
Copy link
Collaborator

gollub commented Nov 4, 2020

@carnil , that's correct. Updated the advisory: "after v1.5.0, before v1.6.1"

@carnil
Copy link

carnil commented Nov 4, 2020

@gollub: Thanks!

@deastoe
Copy link
Contributor Author

deastoe commented Nov 4, 2020

@carnil, @gollub, I believe this also affects v1.4.1:

RAND_pseudo_bytes((unsigned char *) &session_id, sizeof(session_id));

I will update the advisory.

In fact, it looks like v1.4.1 also does not check for a failure of getrandom(). This was fixed in v1.5.1 (d5ea51f)

@deastoe
Copy link
Contributor Author

deastoe commented Nov 4, 2020

In fact, it looks like v1.4.1 also does not check for a failure of getrandom().

However in 1.4.1 getrandom() is only used when not built with libcrypto, and 1.4.1 cannot be built out-of-the-box without libcrypto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants