16

Does using reflection to scrub a String make using String as safe as using char[] for passwords?

From a security aspect, it is generally considered best practice to use char[] for storing/passing passwords, because one can zero-out its contents as soon as possible in code, which may be significantly before garbage collection cleans it up and the memory is reused (wiping all trace), limiting the window of time for a memory attack.

However, char[] is not as convenient as String, so it would be handy if one could "scrub" a String if needed, thus making String as safe as char[].

Below is a method that uses reflection to zero-out the fields of String.

Is this method "OK", and does it achieve the goal of making String as safe as char[] for passwords?

public static void scrub(String str) throws NoSuchFieldException, IllegalAccessException {
    Field valueField = String.class.getDeclaredField("value");
    Field offsetField = String.class.getDeclaredField("offset");
    Field countField = String.class.getDeclaredField("count");
    Field hashField = String.class.getDeclaredField("hash");
    valueField.setAccessible(true);
    offsetField.setAccessible(true);
    countField.setAccessible(true);
    hashField.setAccessible(true);
    char[] value = (char[]) valueField.get(str);
    // overwrite the relevant array contents with null chars
    Arrays.fill(value, offsetField.getInt(str), countField.getInt(str), '\0');
    countField.set(str, 0); // scrub password length too
    hashField.set(str, 0); // the hash could be used to crack a password
    valueField.setAccessible(false);
    offsetField.setAccessible(false);
    countField.setAccessible(false);
    hashField.setAccessible(false);
}

Here's a simple test:

String str = "password";
scrub(str);
System.out.println('"' + str + '"');

Output:

""

Note: You may assume that passwords are not String constants and thus calling this method will have no adverse effect on interned Strings.

Also, I have left the method is a fairly "raw" state for simplicity's sake. If I were to use it, I would not declare exceptions thrown (try/catch/ignoring them) and refactor repeated code.

Roman C
  • 49,761
  • 33
  • 66
  • 176
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • Well, it seems to zero out the chars, but isn't that quite dependent on the JVM implementation? – fge Jun 17 '13 at 15:43
  • memory snooping? that's a fantasy attack. why not worry about something more realistic, like NSA snooping your network traffic. – ZhongYu Jun 17 '13 at 15:48
  • Screams "undefined behavior" at me. Good luck with dragons, velociraptors and black holes :) – CodesInChaos Jun 17 '13 at 15:57
  • 3
    Your code depends on the implementation of the String class. And it happens that the Oracle JDK/openJDK from version 7u6 have a different implementation and your `scrub` method would throw an exception. – assylias Jun 17 '13 at 17:56
  • A good implementation of `String` wont have offset. You ought to be a bit more robust (not something that really happens with most reflection code). / "Memory snooping" isn't an entirely unrealistic attack, particularly when you consider swapping/hibernation may place data on physical medium for a surprising length of time. – Tom Hawtin - tackline Jun 17 '13 at 22:06

2 Answers2

7

There are two potential safety concerns:

  1. The String may share its backing array with other Strings; e.g. if the String was created by calling substring on a larger String. So when you zero the entire value array you could be overwriting the state of other strings ... that don't contain passwords.

    The cure is to only zero the part of the backing array that is used by the password string.

  2. The JLS (17.5.3) warns that the effects of using reflection to change final variables is undefined.

    However, the context for this is the Java Memory Model, and the fact that the compiler is allowed to aggressively cache final variables. In this case:

    • you would expect the String to be thread-confined, and

    • you shouldn't be using any of those variables again.

I wouldn't expect either of these to be real problems ... modulo fixing the over-aggressive zeroing of value.

But the real concern is Velociraptors. :-)

I'm puzzled that you would actually bothering to zap passwords like this. When you think about it, what you are protecting against is the possibility that someone can read process memory ... or a core dump or swap file ... to retrieve passwords. But if someone can do that, your system security has to have already been compromised ... cos' those things most likely require root access (or equivalent). And if they have root access they can "debug" your program and catch the passwords before your application zaps them.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • Perhaps the code is overly brutal/simple. What if only the section of the array pointed to was wiped? And only the count set to zero (to remove trace of how long the password is)? – Bohemian Jun 17 '13 at 15:50
  • The major concern, security, is unaffected by this concern. A string could be observed as broken after resetting. – Marko Topolnik Jun 17 '13 at 15:51
  • OK - I've edited the code to only scrub the section of the array actually being used. – Bohemian Jun 17 '13 at 15:56
  • @MarkoTopolnik - it depends. If think that security trumps all other concerns (including correct function of the application) then I guess it doesn't matter if the password zapper trashes other strings too. Otherwise ... you should at least *consider* this. – Stephen C Jun 17 '13 at 15:56
  • I see what you mean -- my comment wasn't about the issue with structural sharing. But another, related danger is lurking: using reflection you commit yourself to a particular String implementation. – Marko Topolnik Jun 17 '13 at 16:01
  • 1
    @MarkoTopolnik - true, though if the (private) representation used by String changed, the most likely result would be a reflection exception in the zapper. – Stephen C Jun 17 '13 at 16:10
1

One argument I have against String is that it's just too easy to inadvertently make a copy. Using strings safely is possible in theory, but the whole library ecosystem is based on the assumption that it's perfectly OK to copy strings. In the end, considering all the restrictions, strings may not be as convenient for this use case as they generally are.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • But one can copy `char[]` too. Although your point is valid, the coding disipline around passwords should be tightly reviewed to ensure copies are not made, regardless of the type. – Bohemian Jun 17 '13 at 15:47