On Thursday 20 July 2006 18:58, Luca wrote:
> Il Wed, Jul 19, 2006 at 10:13:00PM +0200, Rafael J. Wysocki ha scritto:
> > 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.
>
> Yeah, I didn't noticed that use_RSA was set only after the key had been
> loaded succesfully.
>
> > > > + 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.
>
> With a couple of gotos it's possible to remove the two outer if blocks
Yup. Please have a look at the patch I'm going to post in a while.
> (btw, rsa_data_set was leacked):
Yes, it was intentional. If you free the key created out of a data set, you
can't free the data set itself, apparently, or you get a double free
(libgcrypt "feature", I would guess ;-) ).
> --- a/suspend.c 2006-07-20 18:19:10.242010000 +0200
> +++ b/suspend.c 2006-07-20 18:25:15.656847000 +0200
> @@ -983,48 +983,47 @@
> ret = gcry_ac_key_init(&rsa_pub, rsa_hd, GCRY_AC_KEY_PUBLIC,
> rsa_data_set);
>
> - 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);
> + if (ret)
> + goto Free_data;
> +
> + ret = gcry_ac_data_new(&key_set);
> + if (ret) {
> + gcry_ac_key_destroy(rsa_pub);
> + goto Free_data;
> + }
> +
> + 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';
> }
> - gcry_ac_data_destroy(key_set);
> }
> - gcry_ac_key_destroy(rsa_pub);
> - } else {
> - gcry_ac_data_destroy(rsa_data_set);
> + close(rnd_fd);
> }
> + gcry_ac_data_destroy(key_set);
> + gcry_ac_key_destroy(rsa_pub);
>
> +Free_data:
> + gcry_ac_data_destroy(rsa_data_set);
> Free_rsa:
> gcry_ac_close(rsa_hd);
> Close:
>
>
> > >
> > > > + 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. :-(
>
> I meant:
>
> gcry_control(GCRYCTL_DISABLE_SECMEM, 0);
>
> Not really important though ;)
Ah, I haven't found that. Anyway I'm inclined to let libgcrypt behave as it
wants here.
Greetings,
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/suspend-devel
