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

prevent app_get_pass() from revealing cleartext password on syntax error #6218

Closed
wants to merge 6 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented May 10, 2018

When a user of the OpenSSL CLI makes a syntax error with the password argument of any of the openssl apps, the whole argument is echoed on the console for diagnostic purposes. I find this a bit risky since this may reveal sensitive password contents on the screen or in logs.

This PR limits the diagnostic output to at most, e.g., 4 characters and does not echo the argument of the -pass option at all if it does not contain a ':' within the first 5 characters, indicating that a prefix like pass: or file: is missing.

apps/apps.c Outdated Show resolved Hide resolved
@richsalz richsalz added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels May 10, 2018
apps/apps.c Outdated Show resolved Hide resolved
apps/apps.c Show resolved Hide resolved
@mattcaswell mattcaswell added this to the Assessed milestone Jul 13, 2018
@paulidale paulidale removed the approval: review pending This pull request needs review by a committer label Aug 30, 2018
@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Feb 25, 2019
@mattcaswell
Copy link
Member

ping @levitte - you looked at this one once before.

@DDvO
Copy link
Contributor Author

DDvO commented Mar 13, 2019

@levitte, can you approve/merge this?

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 13, 2019
@levitte
Copy link
Member

levitte commented Mar 13, 2019

Squashed into one commit, added some text to the commit message and merged.

62ca156 prevent app_get_pass() from revealing cleartext password on syntax error

@levitte levitte closed this Mar 13, 2019
levitte pushed a commit that referenced this pull request Mar 13, 2019
When the argument for '-pass' was badly formed, that argument got
displayed in full.  This turns out to not be such a good idea if the
user simply forgot to start the argument with 'pass:', or spellt the
prefix incorrectly.  We therefore change the display to say that a
colon is missing or only showing the incorrect prefix.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6218)
@DDvO DDvO deleted the fix-apt_get_pass-reveal branch March 13, 2019 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants