arangodb / arangodb-java-driver

The official ArangoDB Java driver.
Apache License 2.0
201 stars 94 forks source link

passwords should _not_ use `String` type #393

Open nkiesel opened 3 years ago

nkiesel commented 3 years ago

Storing / passing passwords as String is a common security issue because these password strings will remain in the common String pool for a long time. Instead, passwords should use char[] or byte[] as types. I see this mistake in quite a few places in the API, but it all starts at com.arangodb.ArangoDB.Builder#password

brunobarros2093 commented 1 year ago

Hello @rashtao how you doing? Is this issue still open? Can I help you to fix? I would like to contribute to this project

rashtao commented 1 year ago

Hi @obrunojava , thanks for your offer!

In my opinion, the problem cannot be easily fixed, because in the HttpConnection (https://github.com/arangodb/arangodb-java-driver/blob/main/http/src/main/java/com/arangodb/http/HttpConnection.java) the username and password are converted to an Authorization header value and this can only be set in the underlying Vert.x HttpRequest as String, see

https://github.com/arangodb/arangodb-java-driver/blob/2ad5326ecb04db261f2dc352a6092b77044f9789/http/src/main/java/com/arangodb/http/HttpConnection.java#L255

and

https://github.com/vert-x3/vertx-web/blob/4.4/vertx-web-client/src/main/java/io/vertx/ext/web/client/HttpRequest.java#L184

Therefore the string will be anyways interned there. In this case the string it is {user}:{passwd} Base64 encoded, not plain text, but still a vulnerability.

How would you suggest addressing it?

aburmeis commented 10 months ago

@nkiesel Is this really an issue? String literals are put into the pool by the compiler but instances created by a constructor call are not, as far as I know:

assertTrue("abc" == "abc");
assertFalse(new String("abc") == new String("abc"));

Other types use a pool also for other values like Integer.valueOf() but not String except you are using intern() to force a pool instance.

rashtao commented 10 months ago

Even if we could avoid interning the password string, this will be effectively a long-living String in the heap, living until the driver will be used, because we need to send this information along with every request. Or instead of it we would have in the heap the entire HTTP Authorization header string.

From a security standpoint, in my opinion these are vulnerable in the same way to memory dumps attacks. Long-living char[] or byte[] would be vulnerable as well.