From 3ccbb8fe84364f887c32a21d89932acf5870a07c Mon Sep 17 00:00:00 2001 From: Milan Broz Date: Wed, 27 Sep 2017 10:18:38 +0200 Subject: [PATCH] Fix some problems found by Coverity analysis. --- lib/libdevmapper.c | 15 +++++++++------ lib/luks2/luks2_disk_metadata.c | 16 ++++++++++------ lib/luks2/luks2_json_format.c | 2 +- lib/luks2/luks2_json_metadata.c | 2 +- lib/luks2/luks2_keyslot_luks2.c | 20 ++++++++++---------- lib/luks2/luks2_luks1_convert.c | 7 ++++--- lib/utils_device.c | 6 ++---- lib/utils_device_locking.c | 1 + src/integritysetup.c | 6 ++---- 9 files changed, 40 insertions(+), 35 deletions(-) diff --git a/lib/libdevmapper.c b/lib/libdevmapper.c index 92451b86..e85cd108 100644 --- a/lib/libdevmapper.c +++ b/lib/libdevmapper.c @@ -406,7 +406,8 @@ static int cipher_c2dm(const char *org_c, const char *org_i, unsigned tag_size, i = sscanf(tmp, "%" CLENS "[^-]-%" CLENS "s", mode, iv); if (i == 1) { - strncpy(iv, mode, CLEN); + memset(iv, 0, sizeof(iv)); + strncpy(iv, mode, sizeof(iv) - 1); *mode = '\0'; if (snprintf(capi, sizeof(capi), "%s", cipher) < 0) return -EINVAL; @@ -495,7 +496,8 @@ static int cipher_dm2c(char **org_c, char **org_i, const char *c_dm, const char return -ENOMEM; } else *org_i = NULL; - strncpy(capi, tmp, sizeof(capi)); + memset(capi, 0, sizeof(capi)); + strncpy(capi, tmp, sizeof(capi) - 1); } i = sscanf(capi, "%" CLENS "[^(](%" CLENS "[^)])", mode, cipher); @@ -1426,9 +1428,10 @@ static int _dm_query_crypt(uint32_t get_flags, dmd->flags |= CRYPT_ACTIVATE_SUBMIT_FROM_CRYPT_CPUS; else if (sscanf(arg, "integrity:%u:", &val) == 1) { dmd->u.crypt.tag_size = val; - rintegrity = strchr(arg + strlen("integrity:"), ':') + 1; + rintegrity = strchr(arg + strlen("integrity:"), ':'); if (!rintegrity) goto err; + rintegrity++; } else if (sscanf(arg, "sector_size:%u", &val) == 1) { dmd->u.crypt.sector_size = val; } else /* unknown option */ @@ -1825,7 +1828,7 @@ static int _dm_query_integrity(uint32_t get_flags, dmd->u.integrity.sector_size = val; else if (sscanf(arg, "buffer_sectors:%u", &val) == 1) dmd->u.integrity.buffer_sectors = val; - else if (!strncmp(arg, "internal_hash:", 14)) { + else if (!strncmp(arg, "internal_hash:", 14) && !integrity) { str = &arg[14]; arg = strsep(&str, ":"); integrity = strdup(arg); @@ -1855,7 +1858,7 @@ static int _dm_query_integrity(uint32_t get_flags, if (r < 0) goto err; } - } else if (!strncmp(arg, "journal_crypt:", 14)) { + } else if (!strncmp(arg, "journal_crypt:", 14) && !journal_crypt) { str = &arg[14]; arg = strsep(&str, ":"); journal_crypt = strdup(arg); @@ -1863,7 +1866,7 @@ static int _dm_query_integrity(uint32_t get_flags, r = -ENOMEM; goto err; } - } else if (!strncmp(arg, "journal_mac:", 12)) { + } else if (!strncmp(arg, "journal_mac:", 12) && !journal_integrity) { str = &arg[12]; arg = strsep(&str, ":"); journal_integrity = strdup(arg); diff --git a/lib/luks2/luks2_disk_metadata.c b/lib/luks2/luks2_disk_metadata.c index 40de59ff..d5269de0 100644 --- a/lib/luks2/luks2_disk_metadata.c +++ b/lib/luks2/luks2_disk_metadata.c @@ -76,7 +76,7 @@ static int hdr_checksum_calculate(const char *alg, struct luks2_hdr_disk *hdr_di struct crypt_hash *hd = NULL; int r; - if (crypt_hash_init(&hd, alg)) + if (crypt_hash_size(alg) <= 0 || crypt_hash_init(&hd, alg)) return -EINVAL; /* Binary header, csum zeroed. */ @@ -102,6 +102,9 @@ static int hdr_checksum_check(const char *alg, struct luks2_hdr_disk *hdr_disk, struct luks2_hdr_disk hdr_tmp; int r; + if (crypt_hash_size(alg) <= 0) + return -EINVAL; + /* Copy header and zero checksum. */ memcpy(&hdr_tmp, hdr_disk, LUKS2_HDR_BIN_LEN); memset(&hdr_tmp.csum, 0, sizeof(hdr_tmp.csum)); @@ -576,7 +579,7 @@ int LUKS2_disk_hdr_read(struct crypt_device *cd, struct luks2_hdr *hdr, state_hdr1 = HDR_OBSOLETE; } - /* check header with keyslots fit the device */ + /* check header with keyslots to fit the device */ if (state_hdr1 == HDR_OK) hdr_size = LUKS2_hdr_and_areas_size(jobj_hdr1); else if (state_hdr2 == HDR_OK) @@ -641,16 +644,17 @@ int LUKS2_disk_hdr_read(struct crypt_device *cd, struct luks2_hdr *hdr, hdr_from_disk(&hdr_disk1, &hdr_disk2, hdr, 0); hdr->jobj = jobj_hdr1; json_object_put(jobj_hdr2); - return 0; } else if (state_hdr2 == HDR_OK) { hdr_from_disk(&hdr_disk2, &hdr_disk1, hdr, 1); hdr->jobj = jobj_hdr2; json_object_put(jobj_hdr1); - return 0; } - r = (state_hdr1 == HDR_FAIL_IO || state_hdr2 == HDR_FAIL_IO) ? -EIO : -EINVAL; - + /* + * FIXME: should this fail? At least one header was read correctly. + * r = (state_hdr1 == HDR_FAIL_IO || state_hdr2 == HDR_FAIL_IO) ? -EIO : -EINVAL; + */ + return 0; err: log_dbg("LUKS2 header read failed (%d).", r); diff --git a/lib/luks2/luks2_json_format.c b/lib/luks2/luks2_json_format.c index daa99034..abab160b 100644 --- a/lib/luks2/luks2_json_format.c +++ b/lib/luks2/luks2_json_format.c @@ -182,7 +182,7 @@ int LUKS2_generate_hdr( jobj_segment = json_object_new_object(); json_object_object_add(jobj_segment, "type", json_object_new_string("crypt")); if (detached_metadata_device) - offset = alignPayload * sector_size; + offset = (uint64_t)alignPayload * sector_size; else { //FIXME //offset = size_round_up(areas[7].offset + areas[7].length, alignPayload * SECTOR_SIZE); diff --git a/lib/luks2/luks2_json_metadata.c b/lib/luks2/luks2_json_metadata.c index 65eb3ae2..e3f22566 100644 --- a/lib/luks2/luks2_json_metadata.c +++ b/lib/luks2/luks2_json_metadata.c @@ -941,6 +941,7 @@ int LUKS2_hdr_backup(struct crypt_device *cd, struct luks2_hdr *hdr, if (r) { log_err(cd, _("Failed to acquire read lock on device %s.\n"), device_path(crypt_metadata_device(cd))); + crypt_safe_free(buffer); return r; } @@ -969,7 +970,6 @@ int LUKS2_hdr_backup(struct crypt_device *cd, struct luks2_hdr *hdr, log_err(cd, _("Requested header backup file %s already exists.\n"), backup_file); else log_err(cd, _("Cannot create header backup file %s.\n"), backup_file); - close(devfd); crypt_safe_free(buffer); return -EINVAL; } diff --git a/lib/luks2/luks2_keyslot_luks2.c b/lib/luks2/luks2_keyslot_luks2.c index 21104082..d3cbc968 100644 --- a/lib/luks2/luks2_keyslot_luks2.c +++ b/lib/luks2/luks2_keyslot_luks2.c @@ -126,6 +126,7 @@ static int luks2_decrypt_from_storage(char *dst, size_t dstLength, if (r) { log_err(cd, _("Failed to acquire read lock on device %s.\n"), device_path(device)); + crypt_storage_destroy(s); return r; } @@ -143,15 +144,13 @@ static int luks2_decrypt_from_storage(char *dst, size_t dstLength, device_read_unlock(device); - if (r) { - log_err(cd, _("IO error while decrypting keyslot.\n")); - return r; - } - /* Decrypt buffer */ - r = crypt_storage_decrypt(s, 0, dstLength / SECTOR_SIZE, dst); - crypt_storage_destroy(s); + if (!r) + r = crypt_storage_decrypt(s, 0, dstLength / SECTOR_SIZE, dst); + else + log_err(cd, _("IO error while decrypting keyslot.\n")); + crypt_storage_destroy(s); return r; #endif } @@ -314,9 +313,10 @@ static int luks2_keyslot_get_key(struct crypt_device *cd, if (!json_object_object_get_ex(jobj_kdf, "salt", &jobj2)) return -EINVAL; salt_len = LUKS_SALTSIZE; - base64_decode(json_object_get_string(jobj2), - json_object_get_string_len(jobj2), - salt, &salt_len); + if (!base64_decode(json_object_get_string(jobj2), + json_object_get_string_len(jobj2), + salt, &salt_len)) + return -EINVAL; if (salt_len != LUKS_SALTSIZE) return -EINVAL; diff --git a/lib/luks2/luks2_luks1_convert.c b/lib/luks2/luks2_luks1_convert.c index 2c1f5e07..202849c6 100644 --- a/lib/luks2/luks2_luks1_convert.c +++ b/lib/luks2/luks2_luks1_convert.c @@ -138,7 +138,7 @@ static int json_luks1_segment(const struct luks_phdr *hdr_v1, struct json_object json_object_object_add(segment_obj, "type", field); /* offset field */ - number = hdr_v1->payloadOffset * SECTOR_SIZE; + number = (uint64_t)hdr_v1->payloadOffset * SECTOR_SIZE; field = json_object_new_string(uint64_to_str(num, sizeof(num), &number)); if (!field) { @@ -442,6 +442,7 @@ static int move_keyslot_areas(struct crypt_device *cd, off_t offset_from, devfd = device_open(device, O_RDWR); if (devfd == -1) { log_dbg("Cannot open device %s.", device_path(device)); + free(buf); return -EIO; } @@ -521,7 +522,7 @@ int LUKS2_luks1_to_luks2(struct crypt_device *cd, struct luks_phdr *hdr1, struct strncpy(hdr2->checksum_alg, "sha256", LUKS2_CHECKSUM_ALG_L); crypt_random_get(cd, (char*)hdr2->salt1, sizeof(hdr2->salt1), CRYPT_RND_SALT); crypt_random_get(cd, (char*)hdr2->salt2, sizeof(hdr2->salt2), CRYPT_RND_SALT); - strncpy(hdr2->uuid, crypt_get_uuid(cd), LUKS2_UUID_L); + strncpy(hdr2->uuid, crypt_get_uuid(cd), LUKS2_UUID_L-1); /* UUID should be max 36 chars */ hdr2->jobj = jobj; /* @@ -769,7 +770,7 @@ int LUKS2_luks2_to_luks1(struct crypt_device *cd, struct luks2_hdr *hdr2, struct /* FIXME: LUKS1 requires offset == 0 || offset >= luks1_hdr_size */ hdr1->payloadOffset = offset; - strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L); + strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L - 1); /* max 36 chars */ memcpy(hdr1->magic, luksMagic, LUKS_MAGIC_L); diff --git a/lib/utils_device.c b/lib/utils_device.c index 2b542a37..48ec7cbb 100644 --- a/lib/utils_device.c +++ b/lib/utils_device.c @@ -501,10 +501,8 @@ static int device_info(struct crypt_device *cd, int fd = -1, r, flags = 0, real_readonly; uint64_t real_size; - if (!device) { - r = -ENOTBLK; - goto out; - } + if (!device) + return -ENOTBLK; real_readonly = 0; real_size = 0; diff --git a/lib/utils_device_locking.c b/lib/utils_device_locking.c index 3e1443b7..7d547559 100644 --- a/lib/utils_device_locking.c +++ b/lib/utils_device_locking.c @@ -169,6 +169,7 @@ static void release_lock_handle(struct crypt_lock_handle *h) char res[PATH_MAX]; struct stat buf_a, buf_b; + /* coverity[toctou] */ if (S_ISBLK(h->mode) && /* was it block device */ !flock(h->flock_fd, LOCK_EX | LOCK_NB) && /* lock to drop the file */ !resource_by_devno(res, sizeof(res), h->devno, 1) && /* acquire lock resource name */ diff --git a/src/integritysetup.c b/src/integritysetup.c index 13c59cf8..6eae2fad 100644 --- a/src/integritysetup.c +++ b/src/integritysetup.c @@ -106,8 +106,7 @@ static int _read_keys(char **integrity_key, struct crypt_params_integrity *param if (opt_journal_integrity_key_file) { r = _read_mk(opt_journal_integrity_key_file, &journal_integrity_key, opt_journal_integrity_key_size); if (r < 0) { - crypt_safe_free(*integrity_key); - *integrity_key = NULL; + crypt_safe_free(int_key); return r; } params->journal_integrity_key = journal_integrity_key; @@ -117,8 +116,7 @@ static int _read_keys(char **integrity_key, struct crypt_params_integrity *param if (opt_journal_crypt_key_file) { r = _read_mk(opt_journal_crypt_key_file, &journal_crypt_key, opt_journal_crypt_key_size); if (r < 0) { - crypt_safe_free(*integrity_key); - *integrity_key = NULL; + crypt_safe_free(int_key); crypt_safe_free(journal_integrity_key); return r; }