Convert StringBuilder to Secure String

Question

I am trying to encrypt and decrypt my connection string. Below is the code used to encrypt. When I code underwent SSA Fortify, I get the error as below.

xyz.cs stores sensitive data in an insecure manner, making it possible to extract the data via inspecting the heap.

public static int GetSaltSize(byte[] pBytes)
{
      var key = new Rfc2898DeriveBytes(pBytes, pBytes, 1000);
      byte[] ba = key.GetBytes(2);
      StringBuilder sb = new StringBuilder();
      for (int i = 0; i < ba.Length; i++)
      {
        sb.Append(Convert.ToInt32(ba[i]).ToString());
      }
      int saltSize = 0;
      string s = sb.ToString();   // <--- insecure?
      foreach (char c in s)
      {
        int intc = Convert.ToInt32(c.ToString());
        saltSize = saltSize + intc;
      }
      return saltSize;
}

Please let me know if we can convert StringBuilder to SecureString or what can be the solution.

Having the data in a byte array, and then later in a string builder, is *already* having the data in memory and exposed. If your program actually needs to be secure against someone inspecting the machine's memory (which I'm not actually sure is going to be the case here in the first place) then it's already too late at that point. — Servy, Jul 14 '17 at 13:58
Are you looking for a way to make a string secure? I don't think string builder has much to do with it at the end of the day. If you want a secure string it's probably because you are passing it around, but a string builder is only used to build the string and is not what you would pass around right? — Joe_DM, Jul 14 '17 at 14:01
I'd focus less on the warning and more on the fact that this code appears to go through contortions to compute an `int` value that is, as far as I can tell, completely meaningless. Including the use of `Rfc2898DeriveBytes`, passing the same argument for both password and salt. There are secure ways to store sensitive data like a connection string, but this appears to have very little to do with that. — Jeroen Mostert, Jul 14 '17 at 14:16
What is this even doing? You seem to be getting a pair of bytes, converting them to strings and then concatenating the two numbers. You're then adding all these digits up and calling it the SaltSize? Wut? — Chris, Jul 14 '17 at 14:48

Remus Rusanu · Answer 1 · 2017-07-14T14:17:05.133

There is the dedicated SecureString class for storing sensitive data in memory. I guess your code is adding salt size as a header to some serialized representation. You should not do this, use instead DPAPI via ProtectedData class, which can store securely your connection strings and other sensitive information.

When it comes to using the sensitive connection string, I will speculate a bit assuming we're talking about a database connection string (eg. SqlConnection). Then the .Net connection API does not expose a secure method to initialize credentials. Username/password must be presented to database connection classes in plain text. If you're talking about a SQL Server connection, you should use integrated authentication, which does not require any sensistive information in the connection string.

Camilo Terevinto · Answer 2 · 2017-07-14T15:11:56.763

2

Your attempt to not have the values in memory is worse than not having tried at all to secure it, since you are not storing the int once in memory but rather three times:

First time: sb.Append(Convert.ToInt32(ba[i]).ToString());
Second time: sb.ToString();
Third time: Convert.ToInt32(c.ToString());

So you have each value twice and the complete sentence once.

Since the salt does not need to be secret, and hence doesn't its length either, your code should only be:

public static int GetSaltSize(byte[] pBytes)
{
    var key = new Rfc2898DeriveBytes(pBytes, pBytes, 1000);
    byte[] ba = key.GetBytes(2);
    return ba.SelectMany(x => ((int)x).ToString().ToCharArray()).Sum();
}

edited Jul 14 '17 at 15:11

answered Jul 14 '17 at 14:23

Camilo Terevinto

31,141
6
88
120

You can probably just make it `return 16` (assuming the method name is accurate and the result is the size of the desired, still to be computed salt, in bytes). Though realistically, I'd probably take a good look at the rest of the code base and find out that `GetSaltSize` will be eliminated altogether, because it's part of a roll-your-own crypto scheme. – Jeroen Mostert Jul 14 '17 at 14:33
@JeroenMostert I'd have just declared a static field valued 16, but who knows... I just wanted to at least clean up that mess – Camilo Terevinto Jul 14 '17 at 14:34
I am pretty sure the name is not accurate since it adds up the digits of the two bytes it gets back (which is actually slightly different from what the method in this answer does since the original works digit by digit whereas this just adds the two bytes). – Chris Jul 14 '17 at 14:49
@Chris In the question, it goes like `byte -> int -> string -> char -> string -> int`, mine just does `byte -> int` I'd have to test if it's the same result or not, but both the question and this answer fundamentally do the same: **sum the byes obtained** – Camilo Terevinto Jul 14 '17 at 14:59
@CamiloTerevinto: Looking at one byte if the value is 123 then yours will give a result of 123. Theirs will give a result of 1+2+3=6. I suspect both are wrong but they are not the same. – Chris Jul 14 '17 at 15:08
@Chris Ahhh, I see now what you refer to. I'll edit my answer so that it matches that behavior, but I don't consider that to be intended – Camilo Terevinto Jul 14 '17 at 15:10
@CamiloTerevinto: If you are making guesses on what is intended you are a braver person than I. :) – Chris Jul 14 '17 at 15:12

Convert StringBuilder to Secure String

2 Answers2