Password logged in plain text when making form encoded url calls!

[Ruhrpottpatriot]

When making a form encoded request against a server, this library logs the complete URL of the request which may contain sensitive user data. Using HTTPS doesn't prevent this, since the data is logged before encryption.

Sometimes sending data via form encoding is necessary, e.g. when authorizing a a call made against an API secured with OpenId (see http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest for further information). Currently I'm using a POST request alongside with x_www_form_urlencoded_body to send user data from my login form, however logging of the parameters blows my security wide open. So I'm stuck with two options:

1. Accept the potential security risk.
2. Completely remove VaRest as a method to call apis.

Both are not satisfactory, although I'd take 2 any time over 1 without even thinking about it.

The log calls in question are:

• VaRest/Source/VaRestPlugin/Private/VaRestRequestJSON.cpp

  Line 370 in c4e4563

  UE_LOG(LogVaRest, Log, TEXT("Request (url body): %s %s %s"), *HttpRequest->GetVerb(), *HttpRequest->GetURL(), *UrlParams);

• VaRest/Source/VaRestPlugin/Private/VaRestRequestJSON.cpp

  Line 341 in c4e4563

  UE_LOG(LogVaRest, Log, TEXT("Request (urlencoded): %s %s %s"), *HttpRequest->GetVerb(), *HttpRequest->GetURL(), *UrlParams, *StringRequestContent);

Proposition:

1. Remove the logging of the form body on line 370, and on line 341. This removes the major security vulnerability introduced by plain-text logging.
2. Rename "x_www_form_urlencoded_url" to "query_string" and "x_www_form_urlencoded_body" to "form_encoded" (yes, I know, under the hood both are the same). This makes it more clear to the user what he is potentially getting into.

[amar0k]

I've released a BP that encrypts data for this situation or any other sensitive data you want to transmit over a network, it should be on the marketplace within the next 24hrs.

It is called SHA256 Encryption

[Ruhrpottpatriot]

If you're using HTTPS there shouldn't be a need to encrypt your passwords on the client side, that's what's HTTPS is there for in the first place.

This isn't a thing a third party library should attempt to fix, This is a major security vulnerability on VaRests part and MUST be fixed here. Labelling this as "enhancement" and "feature request" says many things about the developers mindset.

Since this hasn't been fixed months after reporting I purged every piece of VaRest code and I'm now using my own system together with Epics built in HTTP(S) pipeline.

[amar0k]

HTTPS should be used as an absolute minimum for sensitive data . You are still vulnerable to Man-in-the-middle attacks through rogue certificates. ALWAYS encrypt your data , its not difficult.

[ufna]

1. Logs are for development. No logs in shipping builds by default. If you enabled logs in shipping builds, you should protect you private data by yourself (it's your own responsibility).

2. Logs are for development. So, it should be comfort to use and debug. So, all the data will be in logs by default.

3. VaRest is completely open-source. Change anything for your own needs.

4. If you're paranoid about sensitive data, you should encrypt it even in memory.

[Ruhrpottpatriot]

You are still vulnerable to Man-in-the-middle attacks through rogue certificates.

By that logic every encryption should be used as an absolute minimum, as I can obtain a key, and perform a successful MitM attack without either party knowing. You have to place a certain trust in your CA, as you have to do with any key exchange mechanism.
Encrypting a password and then sending it via HTTPS doesn't add any additional security, only complexity and potentials for bugs, it's as simple as that (see here, here and here for additional reasoning).

Logs are for development. So, it should be comfort to use and debug. So, all the data will be in logs by default.

Which is bullshit. You need logs in production to, unless you can guarantee that your product will be 100% bug free after shipping (protip: you can't). There's no need to log a password, even in development. If you need to log a query string and check if that's valid, you should write a BuildQueryString method and log from there, where you can easily redact the password (i.e. replace with garbage values) from the log. That way you can log the query string without exposing sensitive data.
For example: IdentityServer has this at one point in it's version history, too and later upgraded to scrub the password.

VaRest is completely open-source. Change anything for your own needs.

And I did, but I can code. The people on the marketplace who trust the developers to do the right thing either can't or are unaware of the problem. Shifting the responsibility to me (or any other developer for that matter) is just stupid.

[ufna]

Fixed now and secured by default, thanks @Ruhrpottpatriot