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

passwords should _not_ use String type #393

Open
nkiesel opened this issue Jun 7, 2021 · 4 comments
Open

passwords should _not_ use String type #393

nkiesel opened this issue Jun 7, 2021 · 4 comments
Assignees

Comments

@nkiesel
Copy link

nkiesel commented Jun 7, 2021

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

@rashtao rashtao self-assigned this Dec 13, 2022
@brunobarros2093
Copy link

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
Copy link
Collaborator

rashtao commented Jun 7, 2023

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

httpRequest.putHeader(HttpHeaders.AUTHORIZATION.toString(), auth);

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
Copy link

aburmeis commented Jan 9, 2024

@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
Copy link
Collaborator

rashtao commented Jan 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants