zfs crypto discuss - Sep 2008 - ZFS Crypto [Prelim] Codereview

Here''s my comments for the preliminary ZFS Crypto review.
	- Dan

Webrev:	http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/

General comments:
DEA-1 - SCCS keywords need to be removed
DEA-2 - Copyright updated


------------------------------------------------------------------
usr/src/lib/libcryptoutil/common/keyfile.c	pkcs11_read_data()


This code in pkcs11_read_data() scares me in that it may poorly-handle some
hard-to-debug EOF (possibly temporary) EOD cases (I''m easily scared).

Given this new code:

 112                 /* reading from special file may need some coaxing *
 . . .
 117                 for (/* */; left > 0; marker += nread, left -= nread) {
 118                         /* keep reading it''s going well */
 119                         nread = read(fd, marker, left);
 120                         if (nread > 0 || (nread == 0 && errno ==
EINTR))
 121                                 continue;
 122 
 123                         /* might have to be good enough for caller */
 124                         if (nread == 0 && errno == EAGAIN)
 125                                 break;
 126 
 127                         /* anything else is an error */
 128                         ret = errno;
 129                         cryptoerror(LOG_STDERR,
 130                             gettext("error reading file %s: %s"),
filename,
 131                             strerror(ret));
 132                         goto error;
 133                 }

DEA-3 read() can return 0 when there''s no data available at the moment.
ret will be set to 0 (or worse yet, some stale errno code).
If ret is 0, pkcs11_read_data() returns 0, with dbuf and dlen untouched.
If ret is not 0, pkcs11_read_data() returns whatever stale error code is there.

I think errno should be set to 0 before line 119.
I think ret should be set to some non-0 value or perhaps the operation
retried--not sure about this.


DEA-4 I think error handling being consolidated at the end of the function is
groovy.
But, to be more paranoid, perhaps line 147 should be changed to never return 0
as a safety net:

147         return (ret);
That is to, say, something like:
147         return (ret != 0 ? ret : -1);

------------------------------------------------------------------

usr/src/lib/libcryptoutil/common/tohexstr.c	hex_to_bytes()

Given this new code:
  90                 if ((ch >= ''0'') && (ch <=
''9''))
  91                         ch -= ''0'';
  92                 else if ((ch >= ''A'') && (ch
<= ''F''))
  93                         ch = ch - ''A'' + 10;
  94                 else if ((ch >= ''a'') && (ch
<= ''f''))
  95                         ch = ch - ''a'' + 10;

A 256-byte lookup table of char, with a some value > 16 for invalid digits
should be more efficient than a cascading if, but maybe this function
isn''t
used often or isn''t used for long data streams.


DEA-5 Setting blen to 0 has no effect, as it''s a local variable
immediately before return:
107                 blen = 0;


------------------------------------------------------------------
usr/src/lib/libzfs/common/libzfs_crypto.c.html	get_passphrase()

DEA-6: typos for Retrieve and restrictions:
1105                 /* Retreive the source value for the current keyscope */
1353          * keysource changes have no restictions.

DEA-7: hexadecimal is misspelled twice in this function (this is user-visible,
not a comment):
176                             gettext("Enter new hexadecmial key
for"),
180                             gettext("Enter hexadecmial key for"),
zc->zc_name);


DEA-8: Grammar: I suggest changing "Failed generate" to "Failed
to generate"
in this user-visible message:
 194                             dgettext(TEXT_DOMAIN, "Failed generate
"
 195                             "key from passphrase."));

Also here:
 217                     dgettext(TEXT_DOMAIN, "Failed generate "
 218                     "key from passphrase."))


DEA-9: I prefer character constants to magic numbers (in use_key_material()):

Change:
 567                 if ((keylen * 2) + 1 == inkeylen && inkey[keylen*2]
== 10)
 585                 if (inkey[inkeylen - 1] == 10)
To:
 567                 if ((keylen * 2) + 1 == inkeylen && inkey[keylen*2]
== ''\n'')
 585                 if (inkey[inkeylen - 1] == ''\n'')


DEA-10: In zfs_change_key() I see duplicate code in zfs_prop_inherit() being
called twice, with the same parameters, and zfs_prop_set() being called twice
with different parameters, when changed_keysource is non-0:

1146         /* Revert keyscope back if change failed */
1147         if (changed_keyscope) {
1148                 const char *keyscope_str;
1149 
1150                 if (src_keyscope == ZPROP_SRC_LOCAL) {
1151                         (void) zfs_prop_index_to_string(ZFS_PROP_KEYSCOPE,
1152                             keyscope, &keyscope_str);
1153                         (void) zfs_prop_set(zhp,
1154                             zfs_prop_to_name(ZFS_PROP_KEYSCOPE),
keyscope_str);
1155                 } else
1156                         (void) zfs_prop_inherit(zhp,
1157                             zfs_prop_to_name(ZFS_PROP_KEYSCOPE));
1158         }
1159 
1160 cleanup:
1161         /* Revert keysource back if change failed */
1162         if (changed_keysource) {
1163                 if (src_keysource == ZPROP_SRC_LOCAL)
1164                         (void) zfs_prop_set(zhp,
1165                             zfs_prop_to_name(ZFS_PROP_KEYSOURCE),
okeysource);
1166                 else
1167                         (void) zfs_prop_inherit(zhp,
1168                             zfs_prop_to_name(ZFS_PROP_KEYSOURCE));
1169         }

------------------------------------------------------------------
usr/src/lib/libzfs/common/libzfs_crypto.c.html	get_passphrase()

DEA-11 Typos: incoming, suitable, property, successfuly, particularly (twice),
asymmetric, retrieve, rewrapping, dnode_setup_crypto_data:

 409          * The incomming wkeybuf also has the iv stored at the start.
 463  * (usually the dataset) key into a form sutiable for storage in a
 464  * proprerty.
 845  * For safe operation we assume that userland has already successfuly 
 903          * (particulary if we ever support wrapping dataset keys 
 904          * with asymetric keys (eg RSA)).
1900          * data in the ARC is an interesting idea - particuarly
1376  * doesn''t provide away to prompt or retreive the old key. 
1354          * the rewraping if the user buffer was modified while we operated
1652  * dnode_seutp_crypto_data

------------------------------------------------------------------
usr/src/uts/common/fs/zfs/arc.c.sdiff.html

DEA-12: Typo: safety
3152          * This probably isn''t needed but it is a useful saftey
net.

------------------------------------------------------------------
usr/src/uts/common/fs/zfs/sys/zio_crypt.h.html

DEA-13 Typos partially persistent, appropriate, converted, value:

  84  * keystatus is partialy persitent and partially temporary.
  86  * If the on disk state is defined we return the appopriate "in
memory"
 131  *      The userland passphrase was convered to a key using PKCS#5 PBE 
 143  *              zic_keydata is the PIN valule

Period needed at end of line 119 (to avoid run-in with line 120):
 119  * The size of the keydata struct element is hardcoded at 1k
 120  * this is the same as the largest PIN that the crypto framework

------------------------------------------------------------------
--
This message posted from opensolaris.org

Dan Anderson wrote:
> 
> DEA-10: In zfs_change_key() I see duplicate code in zfs_prop_inherit()
being called twice,
 > with the same parameters, and zfs_prop_set() being called twice with 
different parameters,
 > when changed_keysource is non-0:
> 
> 1146         /* Revert keyscope back if change failed */
> 1147         if (changed_keyscope) {
> 1148                 const char *keyscope_str;
> 1149 
> 1150                 if (src_keyscope == ZPROP_SRC_LOCAL) {
> 1151                         (void)
zfs_prop_index_to_string(ZFS_PROP_KEYSCOPE,
> 1152                             keyscope, &keyscope_str);
> 1153                         (void) zfs_prop_set(zhp,
> 1154                             zfs_prop_to_name(ZFS_PROP_KEYSCOPE),
keyscope_str);
> 1155                 } else
> 1156                         (void) zfs_prop_inherit(zhp,
> 1157                             zfs_prop_to_name(ZFS_PROP_KEYSCOPE));
> 1158         }
> 1159 
> 1160 cleanup:
> 1161         /* Revert keysource back if change failed */
> 1162         if (changed_keysource) {
> 1163                 if (src_keysource == ZPROP_SRC_LOCAL)
> 1164                         (void) zfs_prop_set(zhp,
> 1165                             zfs_prop_to_name(ZFS_PROP_KEYSOURCE),
okeysource);
> 1166                 else
> 1167                         (void) zfs_prop_inherit(zhp,
> 1168                             zfs_prop_to_name(ZFS_PROP_KEYSOURCE));
> 1169         }
> 
Just on this one.. Are you sure you see duplicate?  I don''t see what 
you''re saying here.. the top half is completely keyscope and the bottom
half is completely keysource..

Tony

No problem.. I can''t count that high the number of times I mixed up 
keysource and keyscope :)

Tony

Dan Anderson wrote:
> Here''s my comments for the preliminary ZFS Crypto review.
> 	- Dan
> 
> Webrev:	http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/
> 
> General comments:
> DEA-1 - SCCS keywords need to be removed
> DEA-2 - Copyright updated
accepted
> 
> 
> ------------------------------------------------------------------
> usr/src/lib/libcryptoutil/common/keyfile.c	pkcs11_read_data()
> 
> 
> This code in pkcs11_read_data() scares me in that it may poorly-handle some
hard-to-debug EOF (possibly temporary) EOD cases (I''m easily scared).
> 
> Given this new code:
> 
>  112                 /* reading from special file may need some coaxing *
>  . . .
>  117                 for (/* */; left > 0; marker += nread, left -=
nread) {
>  118                         /* keep reading it''s going well */
>  119                         nread = read(fd, marker, left);
>  120                         if (nread > 0 || (nread == 0 &&
errno == EINTR))
>  121                                 continue;
>  122 
>  123                         /* might have to be good enough for caller */
>  124                         if (nread == 0 && errno == EAGAIN)
>  125                                 break;
>  126 
>  127                         /* anything else is an error */
>  128                         ret = errno;
>  129                         cryptoerror(LOG_STDERR,
>  130                             gettext("error reading file %s:
%s"), filename,
>  131                             strerror(ret));
>  132                         goto error;
>  133                 }
> 
> DEA-3 read() can return 0 when there''s no data available at the
moment.
> ret will be set to 0 (or worse yet, some stale errno code).
> If ret is 0, pkcs11_read_data() returns 0, with dbuf and dlen untouched.
> If ret is not 0, pkcs11_read_data() returns whatever stale error code is
there.
> 
> I think errno should be set to 0 before line 119.
> I think ret should be set to some non-0 value or perhaps the operation
retried--not sure about this.
Ok.. I''m making some changes in the loop so that if there is a real 
error, it will break or exit and not get a possible stale errno. 
Otherwise it will loop until the buffer is full.
> 
> 
> DEA-4 I think error handling being consolidated at the end of the function
is groovy.
> But, to be more paranoid, perhaps line 147 should be changed to never
return 0 as a safety net:
> 
> 147         return (ret);
> That is to, say, something like:
> 147         return (ret != 0 ? ret : -1);
Originally it was returning 0 or -1, the for the change was so the 
library function returned the errno value.  Then, in this case, zfs or 
zpool can report an error message in the way it saw fit, if at all.
> 
> ------------------------------------------------------------------
> 
> usr/src/lib/libcryptoutil/common/tohexstr.c	hex_to_bytes()
> 
> Given this new code:
>   90                 if ((ch >= ''0'') && (ch
<= ''9''))
>   91                         ch -= ''0'';
>   92                 else if ((ch >= ''A'') &&
(ch <= ''F''))
>   93                         ch = ch - ''A'' + 10;
>   94                 else if ((ch >= ''a'') &&
(ch <= ''f''))
>   95                         ch = ch - ''a'' + 10;
> 
> A 256-byte lookup table of char, with a some value > 16 for invalid
digits
> should be more efficient than a cascading if, but maybe this function
isn''t
> used often or isn''t used for long data streams.
This code, from zfs/zpool point of view, is just converting a hex key 
file to bytes so it can load the key to unlock the dataset or pool.. It 
doesn''t need to be super efficient since we''re converting a 64
bytes
file at the max :)
> 
> 
> DEA-5 Setting blen to 0 has no effect, as it''s a local variable
immediately before return:
> 107                 blen = 0;
> 
Ah, actually I think stumbled across a problem.. it should be *blen = 0;
> 
> ------------------------------------------------------------------
> usr/src/lib/libzfs/common/libzfs_crypto.c.html	get_passphrase()
> 
> DEA-6: typos for Retrieve and restrictions:
> 1105                 /* Retreive the source value for the current keyscope
*/
> 1353          * keysource changes have no restictions.
> 
> DEA-7: hexadecimal is misspelled twice in this function (this is
user-visible, not a comment):
> 176                             gettext("Enter new hexadecmial key
for"),
> 180                             gettext("Enter hexadecmial key
for"), zc->zc_name);
> 
> 
> DEA-8: Grammar: I suggest changing "Failed generate" to
"Failed to generate"
> in this user-visible message:
>  194                             dgettext(TEXT_DOMAIN, "Failed
generate "
>  195                             "key from passphrase."));
> 
> Also here:
>  217                     dgettext(TEXT_DOMAIN, "Failed generate "
>  218                     "key from passphrase."))
> 
accepted..
> 
> DEA-9: I prefer character constants to magic numbers (in
use_key_material()):
> 
> Change:
>  567                 if ((keylen * 2) + 1 == inkeylen &&
inkey[keylen*2] == 10)
>  585                 if (inkey[inkeylen - 1] == 10)
> To:
>  567                 if ((keylen * 2) + 1 == inkeylen &&
inkey[keylen*2] == ''\n'')
>  585                 if (inkey[inkeylen - 1] == ''\n'')
accepted..

> usr/src/lib/libzfs/common/libzfs_crypto.c.html	get_passphrase()
> 
> DEA-11 Typos: incoming, suitable, property, successfuly, particularly
(twice),
> asymmetric, retrieve, rewrapping, dnode_setup_crypto_data:
> 
>  409          * The incomming wkeybuf also has the iv stored at the start.
>  463  * (usually the dataset) key into a form sutiable for storage in a
>  464  * proprerty.
>  845  * For safe operation we assume that userland has already successfuly 
>  903          * (particulary if we ever support wrapping dataset keys 
>  904          * with asymetric keys (eg RSA)).
> 1900          * data in the ARC is an interesting idea - particuarly
> 1376  * doesn''t provide away to prompt or retreive the old key. 
> 1354          * the rewraping if the user buffer was modified while we
operated
> 1652  * dnode_seutp_crypto_data
> 
the above errors were in /usr/src/uts/common/fs/zfs/zio_crypt.c, and 
i''ve fixed them..
> ------------------------------------------------------------------
> usr/src/uts/common/fs/zfs/arc.c.sdiff.html
> 
> DEA-12: Typo: safety
> 3152          * This probably isn''t needed but it is a useful
saftey net.
accepted
> 
> ------------------------------------------------------------------
> usr/src/uts/common/fs/zfs/sys/zio_crypt.h.html
> 
> DEA-13 Typos partially persistent, appropriate, converted, value:
> 
>   84  * keystatus is partialy persitent and partially temporary.
>   86  * If the on disk state is defined we return the appopriate "in
memory"
>  131  *      The userland passphrase was convered to a key using PKCS#5 PBE
>  143  *              zic_keydata is the PIN valule
> 
> Period needed at end of line 119 (to avoid run-in with line 120):
>  119  * The size of the keydata struct element is hardcoded at 1k
>  120  * this is the same as the largest PIN that the crypto framework
> 
accepted