cmpil...@apache.org wrote on Fri, Mar 30, 2012 at 17:12:37 -0000:
> Author: cmpilato
> Date: Fri Mar 30 17:12:36 2012
> New Revision: 1307538
>
> URL: http://svn.apache.org/viewvc?rev=1307538&view=rev
> Log:
> For issue #4145 ("Master passphrase and encrypted credentials cache"),
> begin adding support for some cryptographic routines in Subversion.
>
> NOTE: The code thus far is no where near complete, but I want to get
> this into version control sooner rather than later.
>
> * subversion/libsvn_subr/crypto.h,
> * subversion/libsvn_subr/crypto.c
> New files, with incomplete and as-yet-unused functions in them.
>
> * subversion/svn/main.c
> (crypto_init): New function.
> (main): Call crypto_init().
>
> * build.conf
> (crypto-test): New section (for a new test binary, also added to
> other relevant bits of this configuration file.
>
> * subversion/tests/libsvn_subr
> Add 'crypto-test' to svn:ignores.
>
> * subversion/tests/libsvn_subr/crypto-test.c
> New skeletal test file.
>
> Added: subversion/trunk/subversion/libsvn_subr/crypto.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/crypto.c?rev=1307538&view=auto
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/crypto.c (added)
> +++ subversion/trunk/subversion/libsvn_subr/crypto.c Fri Mar 30 17:12:36 2012
> @@ -0,0 +1,193 @@
> +/*
> + * crypto.c : cryptographic routines
> + *
> + * ====================================================================
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + * ====================================================================
> + */
> +
> +#ifdef APU_HAVE_CRYPTO
> +
> +#include "crypto.h"
> +
> +#include <apr_random.h>
> +
> +
> +static svn_error_t *
> +get_random_bytes(void **rand_bytes,
> + apr_size_t rand_len,
> + apr_pool_t *pool)
> +{
> + apr_random_t *apr_rand;
Shouldn't you reuse this context as much as possible? (instead of
creating a new context every time)
> + apr_status_t apr_err;
> +
> + *rand_bytes = apr_palloc(pool, rand_len);
> + apr_rand = apr_random_standard_new(pool);
Can this call block? I assume it can't, but APR docs aren't explicit.
Do you need to call apr_random_add_entropy() somewhere? From code
inspection the code will just error out if that function hasn't been
called (that function is the only codepath that sets 'g->secure_started'
to '1').
> + apr_err = apr_random_secure_bytes(apr_rand, *rand_bytes, rand_len);
> + if (apr_err != APR_SUCCESS)
> + return svn_error_create(apr_err, NULL, _("Error obtaining random data"));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_crypto__context_create(apr_crypto_t **crypto_ctx,
> + apr_pool_t *pool)
> +{
> + apr_status_t apr_err;
> + const apu_err_t *apu_err = NULL;
> + const apr_crypto_driver_t *driver;
> +
> + /* ### TODO: So much for abstraction. APR's wrappings around NSS
> + and OpenSSL aren't quite as opaque as I'd hoped, requiring us
> + to specify a driver type and then params to the driver. We
> + *could* use APU_CRYPTO_RECOMMENDED_DRIVER for the driver bit,
> + but we'd still have to then dynamically ask APR which driver
> + it used and then figure out the parameters to send to that
> + driver at apr_crypto_make() time. Or maybe I'm just
> + overlooking something... -- cmpilato */
> +
> + apr_err = apr_crypto_get_driver(&driver, "openssl", NULL, &apu_err, pool);
> + if ((apr_err != APR_SUCCESS) || (! driver))
> + return svn_error_createf(apr_err,
> + err_from_apu_err(apr_err, apu_err),
If the local variable APR_ERR has value APR_SUCCESS, you'll be creating
here an svn_error_t object whose APR_ERR member is APR_SUCCESS. Perhaps
return a more useful error code when (driver == NULL && ! apr_err)?
> + _("OpenSSL crypto driver error"));
> +
> + apr_err = apr_crypto_make(crypto_ctx, driver, "engine=openssl", pool);
> + if ((apr_err != APR_SUCCESS) || (! *crypto_ctx))
> + return svn_error_create(apr_err, NULL,
> + _("Error creating OpenSSL crypto context"));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_crypto__encrypt_cstring(unsigned char **ciphertext,
> + apr_size_t *ciphertext_len,
svn_string_t **ciphertext ?
> + const unsigned char **iv,
> + apr_size_t *iv_len,
> + const unsigned char **salt,
> + apr_size_t *salt_len,
> + apr_crypto_t *crypto_ctx,
> + const char *plaintext,
> + const char *secret,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
> + apr_crypto_key_t *key = NULL;
> + const apu_err_t *apu_err = NULL;
Could move this variable to block scope.
> + apr_status_t apr_err;
> + const unsigned char *prefix;
> + apr_crypto_block_t *block_ctx = NULL;
> + apr_size_t block_size = 0, encrypted_len = 0;
> +
> + SVN_ERR_ASSERT(crypto_ctx);
> +
> + /* Generate the salt. */
> + *salt_len = 8;
> + SVN_ERR(get_random_bytes((void **)salt, *salt_len, result_pool));
> +
> + /* Initialize the passphrase. */
> + apr_err = apr_crypto_passphrase(&key, NULL, secret, strlen(secret),
> + *salt, 8 /* salt_len */,
> + APR_KEY_AES_256, APR_MODE_CBC,
> + 1 /* doPad */, 4096, crypto_ctx,
> + scratch_pool);
> + if (apr_err != APR_SUCCESS)
> + {
> + apr_crypto_error(&apu_err, crypto_ctx);
Need to check the return value of apr_crypto_error() before
dereferencing APU_ERR in err_from_apu_err())
> + return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> + _("Error creating derived key"));
> + }
> +
> + /* Generate a 4-byte prefix. */
> + SVN_ERR(get_random_bytes((void **)&prefix, 4, scratch_pool));
> +
> + /* Initialize block encryption. */
> + apr_err = apr_crypto_block_encrypt_init(&block_ctx, iv, key, &block_size,
> + result_pool);
> + if ((apr_err != APR_SUCCESS) || (! block_ctx))
> + {
> + apr_crypto_error(&apu_err, crypto_ctx);
> + return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> + _("Error initializing block encryption"));
> + }
> +
> + /* ### FIXME: We need to actually use the prefix! */
Also, need to avoid adding 1 to strlen() below if it's a multiple of 16.
> +
> + /* Encrypt the block. */
> + apr_err = apr_crypto_block_encrypt(ciphertext, ciphertext_len,
> + (unsigned char *)plaintext,
> + strlen(plaintext) + 1, block_ctx);
> + if (apr_err != APR_SUCCESS)
> + {
> + apr_crypto_error(&apu_err, crypto_ctx);
> + err = svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> + _("Error encrypting block"));
> + goto cleanup;
> + }
> +
> + /* Finalize the block encryption. */
> + apr_err = apr_crypto_block_encrypt_finish(*ciphertext + *ciphertext_len,
> + &encrypted_len, block_ctx);
> + if (apr_err != APR_SUCCESS)
> + {
> + apr_crypto_error(&apu_err, crypto_ctx);
> + err = svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> + _("Error finalizing block encryption"));
> + goto cleanup;
> + }
> +
> + cleanup:
> + apr_crypto_block_cleanup(block_ctx);
> + return err;
> +}
