search

kravietz / pam_tacplus

TACACS+ protocol client library and PAM module in C. This PAM module support authentication, authorization (account management) and accounting (session management)performed using TACACS+ protocol designed by Cisco.
GNU Lesser General Public License v3.0
130 stars 97 forks source link

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

Closed deastoe closed 3 years ago

deastoe commented 3 years ago

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.

gollub commented 3 years ago

LGTM

deastoe commented 3 years ago

I have requested allocation of a CVE ID from MITRE.

kravietz commented 3 years ago

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

carnil commented 3 years ago

CVE-2020-27743 was assigned for this issue.

kravietz commented 3 years ago

Can you review/edit https://github.com/kravietz/pam_tacplus/security/advisories/GHSA-rp3p-jm35-jv76

deastoe commented 3 years ago

Thanks all for progressing this while I was AFK.

carnil commented 3 years ago

@kravietz, @deastoe looking the commit history of the project it looks that the issue only is introduced after 6fac2504657b8d98fcd627d60ebdbffcf0253b81 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 commented 3 years ago

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

carnil commented 3 years ago

@gollub: Thanks!

deastoe commented 3 years ago

@carnil, @gollub, I believe this also affects v1.4.1: https://github.com/kravietz/pam_tacplus/blob/8dddbec2940f99fa4867d6b6a92d8ba10206915e/libtac/lib/header.c#L93 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 (d5ea51ff6a9b74bdc8a9ea7e6758d520f9b9a9fa)

deastoe commented 3 years ago

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.