Verbose param leaks passwords from basic auth

[bwright86]

I'm new to using the module, and noticed that -Verbose will return details from the Invoke-RestMethod call, which includes the username/password in the Body block.

This behavior could lead to leaked passwords for users, either from peers sitting beside you, or from Powershell transcripts capturing output in text files (This is the case for me, as i have to go back and scrub the file).

It would be easy to add a -Verbose:$false in the below text to prevent leaking password details:

VenafiTppPS/VenafiTppPS/Code/Classes/TppSession.ps1

Line 88 in 99bc2c0

$response = Invoke-TppRestMethod @params

There may be other locations that could leak this detail as well, I will post them as I find them.

It looks like there is a push to move to Token Based authentication in the future, which is a good move. But Tokens are still sensitive, so it would probably be helpful to prevent leaking those as well.

I would be curious if there is a use case where returning the verbose detail from this call would be necessary, if there is a case, some mechanism that is difficult to trigger could be implemented to allow it to be returned.

[bwright86]

It looks like this cmdlet would do the same:

VenafiTppPS/VenafiTppPS/Code/Public/New-TppToken.ps1

Line 144 in 99bc2c0

$response = Invoke-TppRestMethod @params

(Not sure if this is the case only for OAuth or if Integrated UseDefaultCredentials would do the same.)

[bwright86]

Looks like these cmdlets as well:

VenafiTppPS/VenafiTppPS/Code/Public/Get-TppCertificate.ps1

Line 164 in 7c7c3df

$params.Body.Add('Password', $plainTextPassword)

VenafiTppPS/VenafiTppPS/Code/Public/Import-TppCertificate.ps1

Line 152 in 6db33c9

$params.Body.Password = $plainTextPassword

[gdbarron]

Hey @bwright86, thanks for submitting this. I've been meaning to do this for a while and just never got around to it 😄

I'd like to keep the verbose output for all the other rest params which I've found to be very helpful. I will look to just hide the private/secret info. Providing the location where the info should be hidden is a big help, thanks. I'll get working on this.

[bwright86]

Hey @gdbarron, sounds good. If you would like a PR for this, I can create one. I ended up putting a patch into the Invoke-TppRestMethod to handle this. If it found hash keys in the Body for ‘password’ or ‘token’, it would create a ‘redactedparams’ hashtable, replace the value with “[Redacted]”, and provide the correct hashtable to ‘Write-Verbose’ and the Invoke call. This worked well, and even caught the scenario where ‘Get-TppCertificate’ passed a password for encrypting the private key. Thanks for the response!

…

On Dec 12, 2020, at 13:04, Greg Brownstein ***@***.***> wrote:  Hey @bwright86, thanks for submitting this. I've been meaning to do this for a while and just never got around to it 😄 I'd like to keep the verbose output for all the other rest params which I've found to be very helpful. I will look to just hide the private/secret info. Providing the location where the info should be hidden is a big help, thanks. I'll get working on this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[gdbarron]

Nice work on the patch! I took a similar approach, but with some differences. I wanted to be able to capture the params on the way in and verbose on the way out so I created a function to handle both. Let me know what you think. https://github.com/gdbarron/VenafiTppPS/compare/hide-verbose-logging-secrets

The code isn't finished yet, but should be mostly functional.

[bwright86]

Cool, I like the idea of splitting the function out to its own cmdlet.

I will pull this down tomorrow and give it a whirl.

Thanks for looking into it!

[gdbarron]

Hey @bwright86. Get a chance to give it a shot? Thanks.

[gdbarron]

Looks like these cmdlets as well:

VenafiTppPS/VenafiTppPS/Code/Public/Get-TppCertificate.ps1

Line 164 in 7c7c3df

$params.Body.Add('Password', $plainTextPassword)

VenafiTppPS/VenafiTppPS/Code/Public/Import-TppCertificate.ps1

Line 152 in 6db33c9

$params.Body.Password = $plainTextPassword

These don't write-verbose directly, but leave it to Invoke-TppRestMethod to write it out so updating that one function covers these.

[gdbarron]

I had missed hiding the Authorization value in the header, but have now added that as well, 16ddae4

[bwright86]

Cool, good catch, I was curious if there were other sensitive values. I haven’t had a chance to try the improvements, I had been running with my patched version, and it worked well. I will be back in the office on Tuesday, so I can give the updates a once over.

…

Sent from my iPhone

On Dec 20, 2020, at 11:12, Greg Brownstein ***@***.***> wrote:  I had missed hiding the Authorization value in the header, but have now added that as well, 16ddae4 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[bwright86]

I pulled down the changes you made, and moved it into a patched version "2.0.5.2" on my side to test. It seems i am still leaking passwords w/ the Verbose parameter.

I'm guessing the issue on my side is around the Regex expressions being used, but i don't have the time currently to dive into the expressions. I am currently using PSv5.1 on my side.

Would it be difficult to walk through the hashtable to look for sensitive data, versus searching through JSON strings w/ RegEx?

[gdbarron]

It's a hashtable going in, but json coming out so went with the approach I did. I'm open to other ideas. If you can let me know what commands you are running I can see if I can replicate. I'm using v7.1, but will test with v5.1 as well.

[gdbarron]

Found the issue. The formatting between v5 and v7 is slightly different where there's an extra space with v5.

# PS v5 has 2 spaces between key:  value
# PS v7 has 1 space between key: value

I've updated the branch. As I said, I'm definitely open to other ideas as I don't love this approach, but it seems better than recursing hashtables, json, etc with nested key/value pairs.

[bwright86]

Cool, I don’t think I was sure of the approach you took, but it is clearer now. I am not sure if the regex engine has changed between PSv5.1 and PSv7.x. I am aware it is a .NET flavor, and since PSv7 uses .NET Core, there could be some differences. I think we can keep with your flow of converting to JSON and then doing a regex replacement.

…

Sent from my iPhone

On Dec 22, 2020, at 15:50, Greg Brownstein ***@***.***> wrote:  It's a hashtable going in, but json coming out so went with the approach I did. I'm open to other ideas. If you can let me know what commands you are running I can see if I can replicate. I'm using v7.1, but will test with v5.1 as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[bwright86]

Wow, that is a great find. So it was an issue in the JSON converter. Wonderful!

…

Sent from my iPhone

On Dec 22, 2020, at 16:11, Greg Brownstein ***@***.***> wrote:  Found the issue. The formatting between v5 and v7 is slightly different where there's an extra space with v5. # PS v5 has 2 spaces between key: value # PS v7 has 1 space between key: value I've updated the branch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.