On Wednesday 19 July 2006 21:38, Luca wrote:
> Il Wed, Jul 19, 2006 at 05:55:17PM +0200, Rafael J. Wysocki ha scritto:
> > The libgcrypt's AES seems to be significantly slower than the openssl's
> > Blowfish, but well. Also IMHO libgcrypt is less convenient than openssl
> > and the documentation sucks.
>
> Agree...
>
> Btw, it should be made very clear that the user has to regenerate his
> RSA key after the update. I think that if the old key is left in place
> the image will be encrypted with a random key, since generate_key()
> will leave key_data->key uninitialized. See below.
No, it won't. The format of the key file has changed entirely. The new code
will not work with and old key file at all.
> Also I find the code hard to read especially due to the "allocate one
> big buffer and use it multiple times for different stuff"... I know that
> mlockall() may cause future allocations to fail but we are talking about
> a couple of pages at most.
That's true, but this memory is also used to pass some data between
different parts of the code.
Well, perhaps we should untangle it, but I'd prefer to do this separately.
> > Index: suspend/keygen.c
> > ===================================================================
> > --- suspend.orig/keygen.c 2006-07-19 17:23:54.000000000 +0200
> > +++ suspend/keygen.c 2006-07-19 17:24:17.000000000 +0200
> > @@ -55,18 +66,51 @@ int main(int argc, char *argv[])
> > } while (len < MIN_KEY_BITS || len > MAX_KEY_BITS);
> >
> > Retry:
> > - printf("Generating %d-bit RSA keys.\n", len);
> > - rsa = RSA_generate_key(len, RSA_F4, NULL, NULL);
> > - if (!rsa) {
> > - fprintf(stderr, "Key generation failed.\n");
> > + printf("Generating %d-bit RSA keys. Please wait.\n", len);
> > + ret = gcry_ac_key_pair_generate(rsa_hd, len, NULL, &rsa_key_pair, NULL);
> > + if (ret) {
> > + fprintf(stderr, "Key generation failed: %s\n",
> > gcry_strerror(ret));
> > ret = EXIT_FAILURE;
> > - goto Free_RSA;
> > + goto Close_RSA;
> > }
> > - if (RSA_check_key(rsa) <= 0) {
> > + printf("Testing the private key. Please wait.\n");
> > + rsa_priv = gcry_ac_key_pair_extract(rsa_key_pair, GCRY_AC_KEY_SECRET);
> > + ret = gcry_ac_key_test(rsa_hd, rsa_priv);
> > + if (ret) {
> > printf("RSA key test failed. Retrying.\n");
> > goto Retry;
> > }
> >
> > + rsa_data_set = gcry_ac_key_data_get(rsa_priv);
> > + if (gcry_ac_data_length(rsa_data_set) != RSA_FIELDS) {
> > + fprintf(stderr, "Wrong number of key fields\n");
> > + ret = -EINVAL;
> > + goto Free_RSA;
> > + }
> > +
> > + /* Convert the key length into bytes */
> > + size = (len + 7) >> 3;
> > + /* Copy the public key components to struct RSA_data */
> > + offset = 0;
> > + for (j = 0; j < RSA_FIELDS_PUB; j++) {
> > + char *str;
> > + size_t s;
>
> Is the order of exponents and primes guaranteed to be always the same?
I think so, although it's not openly documented. 'n' and 'e' always take
slots 0 and 1, because they are the components of the public key
and the way in which libgcrypt extracts the public key seems to require
this. ;-)
> Maybe it's better to get them using their names.
In fact they are, except the first two are assumed to be 'n' and 'e'.
> > +
> > + if (offset + size >= RSA_DATA_SIZE)
> > + goto Free_RSA;
> > +
> > + gcry_ac_data_get_index(rsa_data_set, GCRY_AC_FLAG_COPY, j,
> > + (const char **)&str, &mpi);
> > + gcry_mpi_print(GCRYMPI_FMT_USG, rsa.data + offset,
> > + size, &s, mpi);
> > + rsa.field[j][0] = str[0];
> > + rsa.field[j][1] = '\0';
> > + rsa.size[j] = s;
> > + offset += s;
> > + gcry_mpi_release(mpi);
> > + gcry_free(str);
> > + }
> > +
> > tcgetattr(0, &termios);
> > termios.c_lflag &= ~ECHO;
> > termios.c_lflag |= ICANON | ECHONL;
>
> > --- suspend.orig/suspend.c 2006-07-19 17:23:54.000000000 +0200
> > +++ suspend/suspend.c 2006-07-19 17:24:17.000000000 +0200
> > @@ -521,31 +521,51 @@ int write_image(int snapshot_fd, int res
> > #ifdef CONFIG_ENCRYPT
> > if (encrypt) {
> > if (use_RSA) {
> > - BF_set_key(&handle.key, KEY_SIZE,
> > key_data->key);
> > - memcpy(handle.ivec, key_data->ivec, IVEC_SIZE);
> > - handle.num = 0;
> > - header->image_flags |= IMAGE_ENCRYPTED |
> > IMAGE_USE_RSA;
> > - memcpy(&header->rsa_data, &key_data->rsa_data,
> > - sizeof(struct RSA_data));
> > - memcpy(&header->key_data, key_data + 1,
> > - sizeof(struct encrypted_key));
> > + error = gcry_cipher_setkey(cipher_handle,
> > + key_data->key, KEY_SIZE);
>
> key_data->key is initialized in generate_key() which may fail silently.
No, it's not. key_data is initialized in main() and 'key' is just a field in
there.
> > + if (!error)
> > + error = gcry_cipher_setiv(cipher_handle,
> > + key_data->ivec, CIPHER_BLOCK);
> > +
> > + if (!error) {
> > + struct encrypted_key *key;
> > +
> > + header->image_flags |= IMAGE_ENCRYPTED |
> > + IMAGE_USE_RSA;
> > + memcpy(&header->rsa, &key_data->rsa,
> > + sizeof(struct RSA_data));
> > + key = (struct encrypted_key *)(key_data
> > + 1);
> > + memcpy(&header->key, key,
> > + sizeof(struct encrypted_key));
> > + }
>
> > @@ -918,10 +938,14 @@ static inline void close_swappiness(void
> > #ifdef CONFIG_ENCRYPT
> > static void generate_key(void)
> > {
> > - RSA *rsa;
> > - int fd, rnd_fd;
> > - struct RSA_data *rsa_data;
> > + gcry_ac_handle_t rsa_hd;
> > + gcry_ac_data_t rsa_data_set, key_set;
> > + gcry_ac_key_t rsa_pub;
> > + gcry_mpi_t mpi;
> > + int ret, fd, rnd_fd;
> > + struct RSA_data *rsa;
> > unsigned char *buf;
> > + int j;
> >
> > if (!key_data)
> > return;
> > @@ -930,41 +954,79 @@ static void generate_key(void)
> > if (fd < 0)
> > return;
> >
> > - rsa = RSA_new();
> > - if (!rsa)
> > + rsa = &key_data->rsa;
> > + if (read(fd, rsa, sizeof(struct RSA_data)) <= 0)
> > goto Close;
> >
> > - rsa_data = &key_data->rsa_data;
> > - if (read(fd, rsa_data, sizeof(struct RSA_data)) <= 0)
> > - goto Free_rsa;
> > + ret = gcry_ac_open(&rsa_hd, GCRY_AC_RSA, 0);
> > + if (ret)
> > + goto Close;
> >
> > - buf = rsa_data->data;
> > - rsa->n = BN_mpi2bn(buf, rsa_data->n_size, NULL);
> > - buf += rsa_data->n_size;
> > - rsa->e = BN_mpi2bn(buf, rsa_data->e_size, NULL);
> > - if (!rsa->n || !rsa->e)
> > + buf = rsa->data;
> > + ret = gcry_ac_data_new(&rsa_data_set);
> > + if (ret)
> > goto Free_rsa;
> >
> > - rnd_fd = open("/dev/urandom", O_RDONLY);
> > - if (rnd_fd > 0) {
> > - unsigned short size = KEY_SIZE + IVEC_SIZE;
> > -
> > - if (read(rnd_fd, key_data->key, size) == size) {
> > - struct encrypted_key *enc_key;
> > - int ret;
> > -
> > - enc_key = (struct encrypted_key *)(key_data + 1);
> > - ret = RSA_public_encrypt(size, key_data->key,
> > - enc_key->data, rsa, RSA_PKCS1_PADDING);
> > - if (ret > 0) {
> > - enc_key->size = ret;
> > - use_RSA = 'y';
> > + for (j = 0; j < RSA_FIELDS_PUB; j++) {
> > + size_t s = rsa->size[j];
> > +
> > + gcry_mpi_scan(&mpi, GCRYMPI_FMT_USG, buf, s, NULL);
> > + ret = gcry_ac_data_set(rsa_data_set, GCRY_AC_FLAG_COPY,
> > + rsa->field[j], mpi);
> > + gcry_mpi_release(mpi);
> > + if (ret)
> > + break;
> > +
> > + buf += s;
> > + }
> > + if (!ret)
> > + ret = gcry_ac_key_init(&rsa_pub, rsa_hd, GCRY_AC_KEY_PUBLIC,
> > + rsa_data_set);
>
> Suppose old key file was left in place. gcry_ac_key_init will probably
> fail, since rsa_data_set is filled with "garbage". key_data->key is left
> uninitlialed and will be used for encrypting the image.
No, it won't, because use_RSA hasn't been set so far.
> Easy fix is to propagated the error, if generate_key() fails suspend
> aborts.
As it is, it should fall back to the "dumb" encryption without RSA
(ie. ask for a passphrase and use it to generate the key/ivec).
> > +
> > + if (!ret) {
> > + ret = gcry_ac_data_new(&key_set);
> > + if (!ret) {
> > + rnd_fd = open("/dev/urandom", O_RDONLY);
> > + if (rnd_fd > 0) {
> > + unsigned short size = KEY_SIZE + CIPHER_BLOCK;
> > +
> > + if (read(rnd_fd, key_data->key, size) == size) {
> > + gcry_mpi_scan(&mpi, GCRYMPI_FMT_USG,
> > + key_data->key, size, NULL);
> > + ret = gcry_ac_data_encrypt(rsa_hd, 0,
> > + rsa_pub, mpi, &key_set);
> > + gcry_mpi_release(mpi);
> > + if (!ret) {
> > + struct encrypted_key *key;
> > + char *str;
> > + size_t s;
> > +
> > + key = (struct encrypted_key *)
> > + (key_data + 1);
> > + gcry_ac_data_get_index(key_set,
> > + GCRY_AC_FLAG_COPY, 0,
> > + (const char **)&str,
> > &mpi);
> > +
> > + gcry_free(str);
> > + gcry_mpi_print(GCRYMPI_FMT_USG,
> > + key->data,
> > KEY_DATA_SIZE,
> > + &s, mpi);
> > + gcry_mpi_release(mpi);
> > + key->size = s;
> > + use_RSA = 'y';
> > + }
> > + }
> > + close(rnd_fd);
> > }
> > + gcry_ac_data_destroy(key_set);
> > }
>
> Hum, I'd reorganize the code to reduce the indendation (and the
> wrapping).
Well, I've considered doing this too, but it seems to me the result won't be
very much more readable. Maybe I'll try in the next version.
> > - close(rnd_fd);
> > + gcry_ac_key_destroy(rsa_pub);
> > + } else {
> > + gcry_ac_data_destroy(rsa_data_set);
> > }
> > +
> > Free_rsa:
> > - RSA_free(rsa);
> > + gcry_ac_close(rsa_hd);
> > Close:
> > close(fd);
> > }
> > @@ -1026,6 +1088,13 @@ int main(int argc, char *argv[])
> > }
> > #ifdef CONFIG_ENCRYPT
> > if (encrypt) {
> > + printf("suspend: libgcrypt version: %s\n",
> > + gcry_check_version(NULL));
>
> Btw, gcry_check_version should be used to ensure that the expected
> version of the library is in place. Bebian for example ships at least 3
> different versions (I don't know if/how the API differs).
s/should/could/
It's just a library-initializing routine that additionally may check if the
version is OK. I'd prefer to postpone this check until we have some known-bad
versions.
>
> > + gcry_control(GCRYCTL_INIT_SECMEM, page_size, 0);
>
> You can also disable secure memory, it's nothing more than a mlock()'ed
> area and we call mlockall() anyway.
RSA encryption doesn't work without this. Worse yet, if this is not done,
libgcrypt just makes the whole program exit. :-(
> > + encrypt = !gcry_cipher_open(&cipher_handle, GCRY_CIPHER_AES,
> > + GCRY_CIPHER_MODE_CFB, GCRY_CIPHER_SECURE);
> > + }
> > + if (encrypt) {
> > mem_size -= buffer_size;
> > key_data = (struct key_data *)((char *)mem_pool + mem_size);
> > generate_key();
> > @@ -1138,6 +1207,10 @@ Close_snapshot_fd:
> > Close_resume_fd:
> > close(resume_fd);
> >
> > +#ifdef CONFIG_ENCRYPT
> > + if (encrypt)
> > + gcry_cipher_close(cipher_handle);
> > +#endif
> > free(mem_pool);
> >
> > return ret;
>
> Will test it ASAP and report back ;)
Thanks!
Rafael
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Suspend-devel mailing list
Suspend-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/suspend-devel
