2
\$\begingroup\$

Security assessment of our codebase reported the following vulnerability -

Vulnerability Description

The method x() in xyz.java stores sensitive data in a String object, making it impossible to reliably purge the data from memory.

Vulnerability Risk

Sensitive data (such as passwords, social security numbers, credit card numbers etc) stored in memory can be leaked if memory is not cleared after use. Often, Strings are used store sensitive data, however, since String objects are immutable, removing the value of a String from memory can only be done by the JVM garbage collector. The garbage collector is not required to run unless the JVM is low on memory, so there is no guarantee as to when garbage collection will take place. In the event of an application crash, a memory dump of the application might reveal sensitive data.

Sample code, with comment against line on which vulnerability has been reported -

String expDetails = "";
        // extract expiry year in YYYY
        if (!CommonUtil.isEmpty(paymentDetails.getExpiryYear())) {
            expDetails = paymentDetails.getExpiryYear();
        }
        // expiry month in MM
        String monthStr = "";
        if (!CommonUtil.isEmpty(paymentDetails.getExpiryMonth())) {
            int month = Integer.parseInt(paymentDetails.getExpiryMonth()) + 1;
            expDetails += month > 9 ? String.valueOf(month) : "0" + month; //Vulnerability reported in this line - String.valueOf() operation it seems.
        }

        try {
            reqParams.put("CardNum",
                    encrypt(params[4], paymentDetails.getCardNumber()));
            reqParams.put("expiryDate", encrypt(params[4], expDetails));
            reqParams.put("CVVNum",
                    encrypt(params[4], paymentDetails.getCvvNumber()));
        } catch (Exception e) {

        }

Remediation

Always be sure to clear sensitive data when it is no longer needed. Instead of storing sensitive data in immutable objects like Strings, use byte arrays or character arrays that can be programmatically cleared.

If I alter this code to following, using a byte array, instead of a string -

    byte[] expDetailsNew = null;
    // extract expiry year in YYYY
    if (!CommonUtil.isEmpty(paymentDetails.getExpiryYear())) {
        expDetailsNew = paymentDetails.getExpiryYear().getBytes();
    }
    // expiry month in MM
    String monthStr = "";
    if (!CommonUtil.isEmpty(paymentDetails.getExpiryMonth())) {
        int month = Integer.parseInt(paymentDetails.getExpiryMonth()) + 1;
        if(month>9)
        {
            byte[] combined = new byte[expDetailsNew.length + Integer.toString(month).length()];
            System.arraycopy(expDetailsNew, 0, combined, 0, expDetailsNew.length);
            System.arraycopy(Integer.toString(month).getBytes(), 0, combined, expDetailsNew.length, Integer.toString(month).length());
            expDetailsNew = combined;
        }
    }

    try {
        reqParams.put("CardNum",
                encrypt(params[4], paymentDetails.getCardNumber()));
        reqParams.put("expiryDate", encrypt(params[4], new String(expDetailsNew))); //I still need to do new String() here
        expDetailsNew = null;
        reqParams.put("CVVNum",
                encrypt(params[4], paymentDetails.getCvvNumber()));
    } catch (Exception e) {

    }

I still need to do new String() at one point, as commented above. Does the vulnerability still remain?

Update

The same vulnerability has also been reported in the following segment as well -

String decValue = null;

        if(someCondition)
        {
            decValue = CitruspgEncryptionUtil.decrypt(encText, key); //Not reported here
        }
        .. many else if blocks which assign string value to decValue variable, but not reported on these
       .. 
else if(someOtherCondition)
        {

            decValue = String.valueOf(someMethodReturningStringOutput); //Vulnerability reported due to the String.valueOf() it seems.
        }

It is not clear how the last case only is vulnerable. Also, what would be the fix here.

Update 2

Method encrypt being called from try block of 1st example -

private String encrypt(String pKey, String pData) throws Exception {

        byte[] lKey = Hex.fromString(pKey);
        byte[] lData = Hex.fromString(pData);

        //lKey, lData are then used as such..
    }
\$\endgroup\$
  • \$\begingroup\$ Please provide more context. Is this code in a web application? Where do the parameters come from? \$\endgroup\$ – 200_success Jul 11 at 16:47
  • \$\begingroup\$ Seems a bit pointless - there already are Strings with card details in paymentDetails object, it is impossible to clear them. Also that rule makes more sense in case of encryption keys - and it looks like params[4] is also a String instead of char[]/byte[] \$\endgroup\$ – user158037 Jul 12 at 10:22
1
\$\begingroup\$

I'll attempt to make two points:

  1. Differentiate where Strings are held in a pool; and,
  2. Evaluate the code-duration where the sting value is held (i.e. start-to-finish number lines of code).

Since Java 11 (if I recall correctly), all strings not using Unicode are byte arrays. So the discussion should be about whether the reference to a string is shared (within a pool) and how long the memory of its contents exist.

Code that creates a string without thy key word 'new' (e.g. String monthStr = "";) will create the string reference in a pool shared within the JVM. I do not recommend sensitive data in a shared pool of strings.

Since you are performing string manipulation, I recommend the StringBuilder class. This class uses a character array. You should be able to avoid false-positive code scans with this class. As soon as you are done with the contents of the StringBuilder class you should clear its memory with either: expDetails.replace(0, expDetails.length(), "*"); or expDetails.delete(0, expDetails.length()); If you continue with your byte array solution I recommend implementing code that writes over the array data after you call the .put(...) method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.