Scrypt cannot be used with more than 16 megabytes of memory about openssl HOT 30 OPEN

@randombit @paulidale @levitte @kroeckx thank you

@randombit as I read it the forward action here is as follows:

• Remove the N check as per the errata (feel free to use my test patch if you like, or write your own)
• Add a CONF variable to the configuration file specifying the maximum memory scrypt should use
• Add code in EVP_PBE_scrypt_ex, to query the configuration option in the above to set maxmem appropriately, allow overides from the passed in parameter maxmem, if it is not 0.

Thank you for being willing to undertake this, I'm assigning it to you as a community issue

When you have a changeset ready, please open a PR and link it here

from openssl.

It is good although I'd prefer doing 2) in the form of documentation fixes in INSTALL.md (and perhaps Configure usage printout). I.e., not adding extra -- option. But if others have a different opinion I won't block this.

from openssl.

I'm hesitant to make a change here, given the errata wasn't acted upon, but in the interim, can you confirm that this patch resolves the problem:

diff --git a/providers/implementations/kdfs/scrypt.c b/providers/implementations/kdfs/scrypt.c
index ee2d4a7d32..098df10d2d 100644
--- a/providers/implementations/kdfs/scrypt.c
+++ b/providers/implementations/kdfs/scrypt.c
@@ -461,18 +461,6 @@ static int scrypt_alg(const char *pass, size_t passlen,
         return 0;
     }
 
-    /*
-     * Need to check N: if 2^(128 * r / 8) overflows limit this is
-     * automatically satisfied since N <= UINT64_MAX.
-     */
-
-    if (16 * r <= LOG2_UINT64_MAX) {
-        if (N >= (((uint64_t)1) << (16 * r))) {
-            ERR_raise(ERR_LIB_EVP, EVP_R_MEMORY_LIMIT_EXCEEDED);
-            return 0;
-        }
-    }
-

from openssl.

It still fails but in a slightly different way

C0443CE2517F0000:error:030000AC:digital envelope routines:scrypt_alg:memory limit exceeded:providers/implementations/kdfs/scrypt.c:503:
C0443CE2517F0000:error:030000AB:digital envelope routines:PKCS5_v2_scrypt_keyivgen_ex:illegal scrypt parameters:crypto/asn1/p5_scrypt.c:285:

This is the check if (Blen + Vlen > maxmem) - I checked and maxmem is apparently being set to 32 MB somehow. Unfortunately it seems the command line doesn't offer any way to configure this.

from openssl.

thats...wierd. The default maxmem bytes are 1025*1024^2

Are you passing a OSSL_KDF_PARAM_SCRYPT_MAXMEM param when you do the derivation? what value are you setting there? Thats the only way I could see that value getting reduced like that

from openssl.

No - as far as I know I'm not doing anything special here, literally just on the command line

openssl pkcs8 -in key.pem -passin=pass:Rabbit

from openssl.

I see the problem

When we decrypt the key file, openssl identifies the password based encryption scheme by the key encoded nid (scrypt), but passes 0 as the maxmem parameter, not knowing what other value to use. As such EVP_PBE_scrypt_ex, seeing a value of 0, sets the default maxmem value to SCRYPT_MAX_MEM, which is 32MB

That complicates this. From an API standpoint, this is a non-issue. If you were writing your own application, it would be easy, you would just call EVP_PBE_scrypt_ex passing an appropriate value.

But because this is an application which is decoding a pem file, the call is burried deep in the call stack, and it has no guidance as to what a reasonable value is there

If I rip out the safety check for maxmem entirely it works:

nhorman@fedora:~/git/openssl$ LD_LIBRARY_PATH=$PWD ./apps/openssl pkcs8 -in ~/key.pem -passin=pass:Rabbit
-----BEGIN PRIVATE KEY-----
MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg4RaNK5CuHY3CXr9f
/CdVgOhEurMohrQmWbbLZK4ZInyhRANCAARs2WMV6UMlLjLaoc0Dsdnj4Vlffc9T
t48lJU0RiCzXc280Vg/H5fm1xAP1B7UnIVcBqgDHDcfqWm1h/xSeCHXS
-----END PRIVATE KEY-----

this all works fine. But that destroys all the protections which maxmem offers

I'm sorry, right now, between the errata that never got addressed, and the need to remove the maxmem protections that keep a system from consuming all of ram, I'm not sure this can be addressed

from openssl.

Can I suggest the following which would IMO improve the situation quite a bit

• Remove the check on N/r which is based on the incorrect RFC text. (What you removed in your original suggested patch.) This check (IIUC) actually prevents any usage of scrypt with larger parameters at all. This already would improve the situation a lot IMO (from 16Mb maximum possible N, to 32 Mb)

and possibly also

• For private key decryption, I can understand that you want to keep it bounded to avoid potential DoS, and can't easily change the call stack to let the caller indicate how much memory is acceptable/safe. But is there any possibility of increasing this limit a bit? This seems pretty clearly an arbitrary limit (at least it was set to this value in the original commit a95fb9e with no particular justification afaict) - any chance of increasing it to say 64M or 128M? These days there are machines with 32 MB of L3 cache.

from openssl.

@paulidale @mattcaswell @t8m @levitte
Can I ask you to comment on item 1 above?

There is an errata for RFC 7914 here:
https://www.rfc-editor.org/errata/rfc7914

which was reported, in which it was suggested that the bounds check on N was invalid, but it was never verified or acted upon by the authors. I'm not sure if we have a policy or best practice for handling non-approved errata, but the math makes sense.

Regarding your request above @randombit, I think that can be considered, and I agree, it does seem somewhat arbitrary, but given that I'm not sure what a 'reasonable' limit is. An adjustement seems in order for larger systems, but openssl supports a wide variety of systems, both modern (large x86_64 system), old (VMS), large (again, x86_64 servers), and small (embedded armv7 systems), so it feels like just picking a different value is something of a recipe for kicking the can down the road, until a smaller system says the limit is too big.

As a counter proposal, I'd suggest a api in e_os.h to preform a run time test of the host platform to determine the amount of ram available and set the limit in scrypt to a fraction of that value. @t8m @mattcaswell @levitte thoughts on that as an approach?

from openssl.

from openssl.

@kroeckx thats why I suggested a run time check, not a compile time check. We certainly can't determine a valid memory limit at build time, that would be no better than the arbitrary limit we have now

As to weather or not we can trust this input data, is it reasonable to say that we can always trust the input? I'm not sure how we can assert that the input being passed in is always trusted

from openssl.

from openssl.

yes, this is about a KDF that contains settings, the problem is that the settings get set in certain cases as part of a larger operation (in this case PKCS5 decoding), so the setting has to be done based on input that is not part of the command line

A configuration setting might be reasonable. A static global variable in the scrypt code initalized at build time to -1, gating a call to the NCONF api to look this up would work I think.

@randombit I'm not sure if I have time to do this, are you interested in putting together a PR for this proposal? I'm happy to assign this to you as a community issue

from openssl.

I'd be very reluctant to determine system memory as the basis for the max memory setting. There is simply so much else happening on a device that we're not going to be able to reliably portion a lump off for scrypt. This information will have to come from the calling application.

Something coming from NCONF or the environment would be reasonable. As would making the maximum memory limit build time configurable (and possibly increasing the default). I wouldn't increase the default without a bypass mechanism. OpenSSL runs on a lot of smaller devices.

I don't see a problem with the N/r check being removed, so long as a sanity check remains elsewhere (which it does).

from openssl.

ACK, thanks @paulidale

@randombit do you think you can try add a configuration option?

from openssl.

I agree with @paulidale, there's should be no problem removing the offending check.

from openssl.

Yes I would be willing to do a PR addressing this, using whatever approach the team thinks best.

from openssl.

Hi all - I started looking into this and am not quite sure of the best way to access a configuration value from EVP_PBE_scrypt_ex. The only parameter that looks likely to have access to a configuration state is the OSSL_LIB_CTX, and while I find functions that translate from an NCONF to a lib ctx (NCONF_get0_libctx) I don't see any thing similar for extracting configuration information from a OSSL_LIB_CTX. I'll continue looking into it, but a hint or reference to code doing a similar operation would be helpful.

from openssl.

ugh, thats right, we don't have a way to fetch the context from the loaded config. I think what you need to do is, in CONF_modules_load_file, after the call to NCONF_load, interrogate the appropriate section where the scrypt maxmem value is stored, and save that to a new variable in the libctx struct, which you can then fetch from the ctx in EVP_PBE_scrypt_ex.

@paulidale @levitte does that seem like the right approach to you?

from openssl.

I don't think there is a way to get a NCONF from a lib ctx.

The usual approach is to call CONF_module_add() to add a configuration processing module. This then gets called when the configuration is loaded & it is expected to save the relevant details somewhere. In this case, it would have to be in the libctx (either directly or as a lib ctx based global).

I'm not sure how good an idea doing this would be but that's the usual process.

from openssl.

barring that, is there a better way to avoid having to set a hard coded limit for maxmem here that you can think of?

from openssl.

ugh, thats right, we don't have a way to fetch the context from the loaded config

#8764 was about that (and also separating loading a configuration from using its contents), but it seems no one was interested at the time...

from openssl.

Would it be a reasonable solution to augment OSSL_LIB_CTX_load_config such that in addition to calling CONF_modules_load_file_ex, we add an additional call to NCONF_new_ex and NCONF_load, storing the resultant pointer in libctx? Add a context api to fetch to CONF object as needed and interrogate it where required (in this case EVP_PBE_scrypt_ex) to fetch out the new config variable?

Or perhaps just save off the created CONF object inside CONF_modules_load_file_ex in the same way? Presumably the CONF object is read only so it should be reasonably thread safe

from openssl.

IMO we should use #8764 to discuss the general concept of subsystem configurations and how it should be handled with the everything should be in libctx concept.

from openssl.

I'd be ok with that, but I presume that would block progress on this issue, until such time as this is resolved (more than a year away). That would be unfortunate. That would make me think that an interim solution for this would be reasonable, like the run time ram check proposed earlier.

from openssl.

Is this issue so critical that it needs resolution without thinking properly about the configuration? I.e. we add another hack configuration module into the existing mess.

Could we at least augment the existing EVP config module instead of adding a new module?

from openssl.

Well, I suppose I'll leave that to the reporter to answer, @randombit how critical is this for you?

As to the interim fix, I think its arguably not that worrysome (noting that I'm referring to a run time check internal to the API). We currently hard code a memory limit when using scrypt in this way, and the interim fix would be to determine that limit at run time based on available ram. Its transparent to any calling api, so it disassociates the need for a CONF setting entirely, and is (again arguably) no more or less arbitrary than what we are doing now.

from openssl.

IMO it's reasonable to consider severing this into two or possibly three distinct PRs:

1. Removing the check on N/r, based on the erroneous RFC text, which effectively prohibits use of more than 16 MB of memory. This change seems (as far as I read the thread) to not be contentious. It would be effectively the patch @nhorman posted early in this issue, plus some relevant tests. This already improves the situation greatly in my view (literally, by a factor of 2, at which point we hit the SCRYPT_MAX_MEM limit) and this seems viable to land in a relatively short timeframe.
2. Already pbe_scrypt.c does support configuring SCRYPT_MAX_MEM at compile time, but it does so only via setting that value as a preprocessor definition, which is not really discoverable or easy to do in the course of a build process. Could this be changed such that the configuration happens at build time? That is, adding some option to Configure like --scrypt-max-mem=32, which allows the builder to specify the limit. Even with (3) in place, something like this seems useful since I expect you'll want to apply some limit, for instance if the config file does not specify anything.
3. Some method of controlling the maximum memory at runtime, either via a configuration option or some other means. Here we seem to be in a situation where it's not so clear what the correct or desirable approach is.

I would propose

• I do ASAP prepare PRs for (1) and - if acceptable - (2). These could, I think be reviewed and merged independently.
• Change (3) is deferred until a suitable design/infrastructure for configuration is worked out. I don't have any opinion on how such configuration system should work and am certainly not the right person to be making those changes. I would be however happy to take on the configuration changes relevant to scrypt once doing so is possible.

from openssl.

@paulidale @levitte @t8m thoughts on the above proposal?

from openssl.

I agree with @t8m, document the required define and let this be set by -DSCRYPT_MAX_MEM=

from openssl.