Secure allocator

[janbilek]

No description provided.

[janbilek]

Retracting

[cdunn2001]

(Retracted? But the branch is deleted and re-created? I'll re-open this for now.)

Here are my initial comments:

• There are not many actual changes, but the diff is huge. This should be structured in a way to minimize the diffs in the PR.
  • Moving things to headers should be done as a separate commit, with a separate PR. If it's just a move, mistakes are less likely. (This is something I always harped on at Amazon too. Almost nobody thinks of doing this, but it's enormously helpful. git diff -C -C should find almost no changes for that move-only PR.) If you create such a PR, it should then be possible to rebase your changes onto that cleanly.
  • Where possible, instead of modifying types to show templates, typedef the old name to the template name. Again, fewer diffs to consider.
• But this whole PR seems to be only for std::string. (Is that correct? In theory, you could also wipe the other dynamically allocated objects, but I don't see that being done. And note that data on the stack can only be wiped in their own constructors, which I also don't see done. So you seem to be interested only in strings.)
  • In that case, this whole thing would be simpler with a String template argument to a Value<String> type.
  • And in that case, we might also want Value<String, Number> or Value<String, Integer, Float>.
  • And in that case, we open the possibility of storing JSON numbers without immediate conversion to native types, which would allow them to be infinitely long (which is allowed by the JSON spec) or to satisfy other user requirements (like implicit conversions, which is a frequent complaint). See the win?
• Switching to String templates everywhere is a lot of work. (And Numbers would be even more work.) I don't expect you to do that. (But if you do, I would love you for it.)
• But given the breadth of switching to templates, it might make more sense to switch all our std::string to a macro (JSONCPP_STRING?) and simply make this whole thing a compile-time option. (And btw, many people would like to use a hashmap instead of a red-black tree. That could also be a compile-time option.)

So let's talk about that. Would it be reasonable to expect people to compile with a special option if they want securely-wiped strings?

[cdunn2001]

After thinking about this more, I have another question: What about user interactions? The API includes std::string?

• Should that be templated on the allocator? That seems to argue for either Value<String> as a template argument, or using a JSON_STRING macro in the public API at compile-time.
• Or do we let the user think about how to wipe their own strings? That seems to make this whole exercise a security joke.

I actually worked in security at Amazon, including internal customer data security and GovCould applications, and even more secret stuff. I remember the long trail of blunders inserted into openssl by security "experts". There is no such thing as a security expert. Everybody makes mistakes. The answer is simplicity.

I suggest that you consider a different, simpler JSON library, gason. It's a much smaller code-base, and it has complete control over memory. Strings are simply char* pointers into its own memory. I suggest forking that and making it wipe its memory pages after use. The author might even incorporate your wiper into his upstream. He has been amenable to pull-requests. (And if you want a simpler API, consider the gason-- wrapper.)

For a C++ security application at Amazon (and of course I cannot give you details), we took memory pages from a pre-allocated pool for each request. All memory for that request came from those pages, and we wiped those pages at the end of each request, before returning them to the memory pool. Very simple to analyze.

In my opinion std::string is the problem, and it's the worst crutch in the standard library. Like I said, we would definitely be interested in ValueTemplate<String, Number>, which would be a lot of work. I don't think Value<Traits, Alloc> offers enough advantage to be worth the disturbance.

The good news is that we really don't change JsonCpp much. Your fork is not likely to diverge far. If we make a significant change, we would probably use a new branch with a major version bump. I don't want to discourage you from using your code if it satisfies your particular requirements well.

[dawesc]

Hi Chris, thanks so much for this, we're in the middle of re-working this because as you say the diff was just too massive when it got reviewed internally which is why we opted to remove it. Thanks so much for the comments above, we'll try as hard as possible to go in that direction; with regards to std::string in the API, for backward compatibility it's no issue hopefully because people will be using std::string with the standard allocator. What we're trying to do is use Value<String, Allocator> as the type and then have everywhere else SomeClass; what this means is that we're using typedef's from Value to dictate the usage of string everywhere else. At the moment we've left some as std::string still and not Value::String because we wondered whether for errors you'd like to have them a string still but were concerned that they could then contain errors with sensitive information.

On the numbers, yeah, I'd just wondered who would have secure numbers in JSON, there just isn't space in a number for it to be particularly sensitive but i guess you could have an array of numbers for example representing a password or key or ... so will have a good think about that too. I will need to have a careful think about the union and how it will be affected by using allocators.

We will hopefully get this finished by the end of the weekend to as you say work through the commits in a more structured approach.

Thanks a lot, christopher