[PATCH] Fix string to integer conversion for occurences in source3
Jeremy Allison
jra at samba.org
Thu Jan 10 20:09:28 UTC 2019
On Thu, Jan 10, 2019 at 06:54:51PM +0100, swen via samba-technical wrote:
> This is patchset is the successor of the mail thread
> [PATCH] [tldap] check for successful string conversion
> which I had with Volker today.
>
> As stated in the subject this patch-set updates all occurences of
> the usage strtoul(l) to verify the result is valid.
>
> Please review and push if happy.
>
> Thanks for your support in advance.
Hi Swen,
I followed the conversation with Volker on the strtoul(l)
saga, and in the epic words of Futurama, you are "Technically
Correct, The Best Kind of Correct" :-).
https://www.youtube.com/watch?v=hou0lU8WMgo
Having said that there are a bunch of issues with
this change that we need to fix.
1). nttime_from_string() is actually an unused function.
diff --git a/source3/lib/time.c b/source3/lib/time.c
index 30ad1ec9a01..09c5b0656f6 100644
--- a/source3/lib/time.c
+++ b/source3/lib/time.c
@@ -42,6 +42,7 @@
*/
NTTIME nttime_from_string(const char *s)
{
+ errno = 0;
return strtoull(s, NULL, 0);
}
Rather than fix, just delete it !
2). Setting errno=0 inside VFS code is fraught with
danger. Your changes indirectly do this by changing
conv_str_size() to set errno to zero as a side effect
which is called by some of the VFS functions.
Some of the VFS returns depend on errno coming out
correctly from a call (yes I know, they're bad interfaces but
they are what they are). Normally this is done by
ensuring that the errno-setting function is the
last one called before exit from the VFS, but any
change that explicitly sets errno can cause subtle
hard to track bugs.
So what I'd prefer is for you to create wrapper
functions that preserve an existing errno:
strtoul_err()
and:
strtoull_err()
Here is a sample implementation (not compiled or tested I'm
afraid :-):
unsigned long int strtoul_err(const char *nptr, char **endptr, int base, int *perr)
{
unsigned long int val;
int saved_errno = errno;
*perr = 0;
errno = 0;
val = strtoul(nptr, endptr, base);
if (errno != 0) {
*perr = errno;
}
errno = saved_errno;
return val;
}
unsigned long long int strtoull_err(const char *nptr, char **endptr, int base, int *perr)
{
unsigned long long int val;
int saved_errno = errno;
*perr = 0;
errno = 0;
val = strtoull(nptr, endptr, base);
if (errno != 0) {
*perr = errno;
}
errno = saved_errno;
return val;
}
Put them in lib/uil/util.c
Then it should be easy to just call these instead of the horrid
strtoul() and strtoull() functions and know that they're safe
to use anywhere. Will mean less code changes, and just the
additional &err parameter being checked.
Does that sound like a plan ?
> P.S.: My comment/question about strings representing negative values is
> still vital...so please comment !
> From 902b64aac9a7cbbf58982ac2cf10a9484190abe3 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 09:44:00 +0100
> Subject: [PATCH 1/7] lib: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/lib/interface.c | 7 +++++--
> source3/lib/messages_dgm.c | 9 ++++-----
> source3/lib/namemap_cache.c | 6 ++++--
> source3/lib/sysquotas.c | 5 +++++
> source3/lib/time.c | 1 +
> source3/lib/tldap_util.c | 9 +++++++++
> source3/lib/util_str.c | 3 ++-
> 7 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/source3/lib/interface.c b/source3/lib/interface.c
> index a3bc5d24e91..3c7cdc4caa6 100644
> --- a/source3/lib/interface.c
> +++ b/source3/lib/interface.c
> @@ -516,8 +516,11 @@ static void interpret_interface(char *token)
> }
> } else {
> char *endp = NULL;
> - unsigned long val = strtoul(p, &endp, 0);
> - if (p == endp || (endp && *endp != '\0')) {
> + unsigned long val;
> +
> + errno = 0;
> + val = strtoul(p, &endp, 0);
> + if (p == endp || (endp && *endp != '\0') || (errno != 0)) {
> DEBUG(2,("interpret_interface: "
> "can't determine netmask value from %s\n",
> p));
> diff --git a/source3/lib/messages_dgm.c b/source3/lib/messages_dgm.c
> index af12be8d82e..f1b81a53c5b 100644
> --- a/source3/lib/messages_dgm.c
> +++ b/source3/lib/messages_dgm.c
> @@ -1457,13 +1457,12 @@ static int messaging_dgm_read_unique(int fd, uint64_t *punique)
> }
> buf[rw_ret] = '\0';
>
> + errno = 0;
> unique = strtoull(buf, &endptr, 10);
> - if ((unique == 0) && (errno == EINVAL)) {
> - return EINVAL;
> - }
> - if ((unique == ULLONG_MAX) && (errno == ERANGE)) {
> - return ERANGE;
> + if (errno != 0) {
> + return errno;
> }
> +
> if (endptr[0] != '\n') {
> return EINVAL;
> }
> diff --git a/source3/lib/namemap_cache.c b/source3/lib/namemap_cache.c
> index fa179517f9f..677eaf2c509 100644
> --- a/source3/lib/namemap_cache.c
> +++ b/source3/lib/namemap_cache.c
> @@ -123,8 +123,9 @@ static void namemap_cache_find_sid_parser(
> return;
> }
>
> + errno = 0;
> type = strtoul(typebuf, &endptr, 10);
> - if (*endptr != '\0') {
> + if ((*endptr != '\0') || (errno != 0)) {
> return;
> }
> if ((type == ULONG_MAX) && (errno == ERANGE)) {
> @@ -276,8 +277,9 @@ static void namemap_cache_find_name_parser(
> return;
> }
>
> + errno = 0;
> type = strtoul(typebuf, &endptr, 10);
> - if (*endptr != '\0') {
> + if ((*endptr != '\0') || (errno != 0)) {
> return;
> }
> if ((type == ULONG_MAX) && (errno == ERANGE)) {
> diff --git a/source3/lib/sysquotas.c b/source3/lib/sysquotas.c
> index 9b2d37b8375..6a7547d450f 100644
> --- a/source3/lib/sysquotas.c
> +++ b/source3/lib/sysquotas.c
> @@ -282,7 +282,12 @@ static int command_get_quota(const char *path, enum SMB_QUOTA_TYPE qtype, unid_t
>
> /* we need to deal with long long unsigned here, if supported */
>
> + errno = 0;
> dp->qflags = strtoul(line, &p2, 10);
> + if (errno != 0) {
> + goto invalid_param;
> + }
> +
> p = p2;
> while (p && *p && isspace(*p)) {
> p++;
> diff --git a/source3/lib/time.c b/source3/lib/time.c
> index 30ad1ec9a01..09c5b0656f6 100644
> --- a/source3/lib/time.c
> +++ b/source3/lib/time.c
> @@ -42,6 +42,7 @@
> */
> NTTIME nttime_from_string(const char *s)
> {
> + errno = 0;
> return strtoull(s, NULL, 0);
> }
>
> diff --git a/source3/lib/tldap_util.c b/source3/lib/tldap_util.c
> index 508c6c02f80..d8a65c4e2d8 100644
> --- a/source3/lib/tldap_util.c
> +++ b/source3/lib/tldap_util.c
> @@ -395,7 +395,16 @@ bool tldap_pull_uint64(struct tldap_message *msg, const char *attr,
> DEBUG(10, ("Could not find attribute %s\n", attr));
> return false;
> }
> +
> + errno = 0;
> result = strtoull(str, NULL, 10);
> + if (errno != 0) {
> + DBG_DEBUG("Attribute conversion failed (%s)\n",
> + strerror(errno));
> + TALLOC_FREE(str);
> + return false;
> + }
> +
> TALLOC_FREE(str);
> *presult = result;
> return true;
> diff --git a/source3/lib/util_str.c b/source3/lib/util_str.c
> index 8568af46c17..920c1732782 100644
> --- a/source3/lib/util_str.c
> +++ b/source3/lib/util_str.c
> @@ -856,9 +856,10 @@ uint64_t conv_str_size(const char * str)
> return 0;
> }
>
> + errno = 0;
> lval = strtoull(str, &end, 10 /* base */);
>
> - if (end == NULL || end == str) {
> + if (end == NULL || end == str || errno != 0) {
> return 0;
> }
>
> --
> 2.20.1
>
>
> From 8da1825c141bf1baa3d12a67492cc95e469828f0 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 17:44:44 +0100
> Subject: [PATCH 2/7] groupdb: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/groupdb/mapping.c | 5 +++++
> source3/groupdb/mapping_tdb.c | 6 ++++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/source3/groupdb/mapping.c b/source3/groupdb/mapping.c
> index 43722e777d4..77ea86d5bef 100644
> --- a/source3/groupdb/mapping.c
> +++ b/source3/groupdb/mapping.c
> @@ -243,8 +243,13 @@ int smb_create_group(const char *unix_group, gid_t *new_gid)
>
> nread = read(fd, output, sizeof(output)-1);
> if (nread > 0) {
> + errno = 0;
> output[nread] = '\0';
> *new_gid = (gid_t)strtoul(output, NULL, 10);
> + if (errno != 0) {
> + *new_gid = 0;
> + return errno;
> + }
> }
>
> close(fd);
> diff --git a/source3/groupdb/mapping_tdb.c b/source3/groupdb/mapping_tdb.c
> index d6a06ef199b..391ee9e8d1f 100644
> --- a/source3/groupdb/mapping_tdb.c
> +++ b/source3/groupdb/mapping_tdb.c
> @@ -974,8 +974,9 @@ static int convert_ldb_record(TDB_CONTEXT *ltdb, TDB_DATA key,
> /* we ignore unknown or uninteresting attributes
> * (objectclass, etc.) */
> if (strcasecmp_m(name, "gidNumber") == 0) {
> + errno = 0;
> map->gid = strtoul(val, &q, 10);
> - if (*q) {
> + if (*q || errno != 0) {
> errno = EIO;
> goto failed;
> }
> @@ -985,8 +986,9 @@ static int convert_ldb_record(TDB_CONTEXT *ltdb, TDB_DATA key,
> goto failed;
> }
> } else if (strcasecmp_m(name, "sidNameUse") == 0) {
> + errno = 0;
> map->sid_name_use = strtoul(val, &q, 10);
> - if (*q) {
> + if (*q || errno != 0) {
> errno = EIO;
> goto failed;
> }
> --
> 2.20.1
>
>
> From 5f27a2ef011bdf8dc18a759c85c25bbf30657b5e Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 17:57:45 +0100
> Subject: [PATCH 3/7] utils: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/utils/net_idmap.c | 3 ++-
> source3/utils/net_registry.c | 14 +++++++++++++-
> source3/utils/net_rpc_registry.c | 9 ++++++++-
> source3/utils/net_sam.c | 5 ++++-
> source3/utils/pdbedit.c | 9 +++++++--
> source3/utils/regedit_dialog.c | 8 ++++++--
> 6 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/source3/utils/net_idmap.c b/source3/utils/net_idmap.c
> index b49d5f43381..e4af80a9e0b 100644
> --- a/source3/utils/net_idmap.c
> +++ b/source3/utils/net_idmap.c
> @@ -629,6 +629,7 @@ static bool parse_uint32(const char *str, uint32_t *result)
> unsigned long val;
> char *endptr;
>
> + errno = 0;
> val = strtoul(str, &endptr, 10);
>
> if (str == endptr) {
> @@ -637,7 +638,7 @@ static bool parse_uint32(const char *str, uint32_t *result)
> if (*endptr != '\0') {
> return false;
> }
> - if ((val == ULONG_MAX) && (errno == ERANGE)) {
> + if (errno != 0) {
> return false;
> }
> if ((val & UINT32_MAX) != val) {
> diff --git a/source3/utils/net_registry.c b/source3/utils/net_registry.c
> index 01a36b20e7c..beb35e0d47c 100644
> --- a/source3/utils/net_registry.c
> +++ b/source3/utils/net_registry.c
> @@ -509,7 +509,14 @@ static int net_registry_setvalue(struct net_context *c, int argc,
> }
>
> if (strequal(argv[2], "dword")) {
> - uint32_t v = strtoul(argv[3], NULL, 10);
> + uint32_t v;
> +
> + errno = 0;
> + v = strtoul(argv[3], NULL, 10);
> + if (errno != 0) {
> + ret = errno;
> + goto done;
> + }
> value.type = REG_DWORD;
> value.data = data_blob_talloc(ctx, NULL, 4);
> SIVAL(value.data.data, 0, v);
> @@ -641,7 +648,12 @@ static int net_registry_increment(struct net_context *c, int argc,
>
> state.increment = 1;
> if (argc == 3) {
> + errno = 0;
> state.increment = strtoul(argv[2], NULL, 10);
> + if (errno != 0) {
> + ret = errno;
> + goto done;
> + }
> }
>
> status = g_lock_do(string_term_tdb_data("registry_increment_lock"),
> diff --git a/source3/utils/net_rpc_registry.c b/source3/utils/net_rpc_registry.c
> index 19b73fd7042..29c8fcd8868 100644
> --- a/source3/utils/net_rpc_registry.c
> +++ b/source3/utils/net_rpc_registry.c
> @@ -603,7 +603,14 @@ static NTSTATUS rpc_registry_setvalue_internal(struct net_context *c,
> }
>
> if (strequal(argv[2], "dword")) {
> - uint32_t v = strtoul(argv[3], NULL, 10);
> + uint32_t v;
> +
> + errno = 0;
> + v = strtoul(argv[3], NULL, 10);
> + if (errno != 0) {
> + goto error;
> + }
> +
> value.type = REG_DWORD;
> value.data = data_blob_talloc(mem_ctx, NULL, 4);
> SIVAL(value.data.data, 0, v);
> diff --git a/source3/utils/net_sam.c b/source3/utils/net_sam.c
> index 5a9d1792d51..5cf9565f397 100644
> --- a/source3/utils/net_sam.c
> +++ b/source3/utils/net_sam.c
> @@ -500,9 +500,12 @@ static int net_sam_policy_set(struct net_context *c, int argc, const char **argv
> value = -1;
> }
> else {
> + errno = 0;
> value = strtoul(argv[1], &endptr, 10);
>
> - if ((endptr == argv[1]) || (endptr[0] != '\0')) {
> + if ((endptr == argv[1]) ||
> + (endptr[0] != '\0') ||
> + (errno != 0)) {
> d_printf(_("Unable to set policy \"%s\"! Invalid value "
> "\"%s\".\n"),
> account_policy, argv[1]);
> diff --git a/source3/utils/pdbedit.c b/source3/utils/pdbedit.c
> index 585f1bfada3..478a5892644 100644
> --- a/source3/utils/pdbedit.c
> +++ b/source3/utils/pdbedit.c
> @@ -598,9 +598,14 @@ static int set_user_info(const char *username, const char *fullname,
> time_t value = get_time_t_max();
>
> if (strcmp(kickoff_time, "never") != 0) {
> - uint32_t num = strtoul(kickoff_time, &endptr, 10);
> + uint32_t num;
>
> - if ((endptr == kickoff_time) || (endptr[0] != '\0')) {
> + errno = 0;
> + num = strtoul(kickoff_time, &endptr, 10);
> +
> + if ((endptr == kickoff_time) ||
> + (endptr[0] != '\0') ||
> + (errno != 0)) {
> fprintf(stderr, "Failed to parse kickoff time\n");
> return -1;
> }
> diff --git a/source3/utils/regedit_dialog.c b/source3/utils/regedit_dialog.c
> index dcce66bf208..c1b3cda5068 100644
> --- a/source3/utils/regedit_dialog.c
> +++ b/source3/utils/regedit_dialog.c
> @@ -1032,6 +1032,7 @@ bool dialog_section_text_field_get_uint(struct dialog_section *section,
> bool rv;
> const char *buf;
> char *endp;
> + unsigned long long tmp_out;
> struct dialog_section_text_field *text_field =
> talloc_get_type_abort(section, struct dialog_section_text_field);
>
> @@ -1041,12 +1042,15 @@ bool dialog_section_text_field_get_uint(struct dialog_section *section,
> if (buf == NULL) {
> return false;
> }
> - *out = strtoull(buf, &endp, 0);
> +
> + errno = 0;
> + tmp_out = strtoull(buf, &endp, 0);
> rv = true;
> - if (endp == buf || endp == NULL || endp[0] != '\0') {
> + if (endp == buf || endp == NULL || endp[0] != '\0' || errno != 0) {
> rv = false;
> }
>
> + *out = tmp_out;
> return rv;
> }
>
> --
> 2.20.1
>
>
> From 1db9fe6391049e555c3b7e12ae402ae6ddce2a1e Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 18:24:16 +0100
> Subject: [PATCH 4/7] passdb: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/passdb/account_pol.c | 8 ++++-
> source3/passdb/pdb_ldap.c | 61 +++++++++++++++++++++++++++++++-----
> source3/passdb/pdb_tdb.c | 4 +++
> 3 files changed, 65 insertions(+), 8 deletions(-)
>
> diff --git a/source3/passdb/account_pol.c b/source3/passdb/account_pol.c
> index e566eca78eb..0d748a12dfd 100644
> --- a/source3/passdb/account_pol.c
> +++ b/source3/passdb/account_pol.c
> @@ -456,7 +456,13 @@ bool cache_account_policy_get(enum pdb_policy_type type, uint32_t *value)
> }
>
> if (gencache_get(cache_key, talloc_tos(), &cache_value, NULL)) {
> - uint32_t tmp = strtoul(cache_value, NULL, 10);
> + uint32_t tmp;
> +
> + errno = 0;
> + tmp = strtoul(cache_value, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> *value = tmp;
> ret = True;
> }
> diff --git a/source3/passdb/pdb_ldap.c b/source3/passdb/pdb_ldap.c
> index 7f8903ba96d..238a18b13e3 100644
> --- a/source3/passdb/pdb_ldap.c
> +++ b/source3/passdb/pdb_ldap.c
> @@ -995,7 +995,12 @@ static bool init_sam_from_ldap(struct ldapsam_privates *ldap_state,
> ctx);
> if (temp) {
> /* We've got a uid, feed the cache */
> + errno = 0;
> unix_pw.pw_uid = strtoul(temp, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert UID\n");
> + goto fn_exit;
> + }
> have_uid = true;
> }
> temp = smbldap_talloc_single_attribute(
> @@ -1005,7 +1010,12 @@ static bool init_sam_from_ldap(struct ldapsam_privates *ldap_state,
> ctx);
> if (temp) {
> /* We've got a uid, feed the cache */
> + errno = 0;
> unix_pw.pw_gid = strtoul(temp, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert GID\n");
> + goto fn_exit;
> + }
> have_gid = true;
> }
> unix_pw.pw_gecos = smbldap_talloc_single_attribute(
> @@ -2928,7 +2938,12 @@ static NTSTATUS ldapsam_enum_group_memberships(struct pdb_methods *methods,
> ret = NT_STATUS_INTERNAL_DB_CORRUPTION;
> goto done;
> }
> + errno = 0;
> primary_gid = strtoul(gidstr, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert GID\n");
> + goto done;
> + }
> break;
> default:
> DEBUG(1, ("found more than one account with the same user name ?!\n"));
> @@ -2996,9 +3011,10 @@ static NTSTATUS ldapsam_enum_group_memberships(struct pdb_methods *methods,
> str, sizeof(str)-1))
> continue;
>
> + errno = 0;
> gid = strtoul(str, &end, 10);
>
> - if (PTR_DIFF(end, str) != strlen(str))
> + if (PTR_DIFF(end, str) != strlen(str) || errno != 0)
> goto done;
>
> if (gid == primary_gid) {
> @@ -4923,6 +4939,7 @@ static NTSTATUS ldapsam_get_new_rid(struct ldapsam_privates *priv,
> char *value;
> int rc;
> uint32_t nextRid = 0;
> + uint32_t tmp;
> const char *dn;
>
> TALLOC_CTX *mem_ctx;
> @@ -4959,21 +4976,33 @@ static NTSTATUS ldapsam_get_new_rid(struct ldapsam_privates *priv,
> value = smbldap_talloc_single_attribute(priv2ld(priv), entry,
> "sambaNextRid", mem_ctx);
> if (value != NULL) {
> - uint32_t tmp = (uint32_t)strtoul(value, NULL, 10);
> + errno = 0;
> + tmp = (uint32_t)strtoul(value, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> nextRid = MAX(nextRid, tmp);
> }
>
> value = smbldap_talloc_single_attribute(priv2ld(priv), entry,
> "sambaNextUserRid", mem_ctx);
> if (value != NULL) {
> - uint32_t tmp = (uint32_t)strtoul(value, NULL, 10);
> + errno = 0;
> + tmp = (uint32_t)strtoul(value, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> nextRid = MAX(nextRid, tmp);
> }
>
> value = smbldap_talloc_single_attribute(priv2ld(priv), entry,
> "sambaNextGroupRid", mem_ctx);
> if (value != NULL) {
> - uint32_t tmp = (uint32_t)strtoul(value, NULL, 10);
> + errno = 0;
> + tmp = (uint32_t)strtoul(value, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> nextRid = MAX(nextRid, tmp);
> }
>
> @@ -5048,6 +5077,7 @@ static bool ldapsam_sid_to_id(struct pdb_methods *methods,
> NULL };
> LDAPMessage *result = NULL;
> LDAPMessage *entry = NULL;
> + unsigned long tmp_id;
> bool ret = False;
> char *value;
> int rc;
> @@ -5106,7 +5136,13 @@ static bool ldapsam_sid_to_id(struct pdb_methods *methods,
> goto done;
> }
>
> - id->id = strtoul(gid_str, NULL, 10);
> + errno = 0;
> + tmp_id = strtoul(gid_str, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> +
> + id->id = tmp_id;
> id->type = ID_TYPE_GID;
> ret = True;
> goto done;
> @@ -5122,7 +5158,12 @@ static bool ldapsam_sid_to_id(struct pdb_methods *methods,
> goto done;
> }
>
> - id->id = strtoul(value, NULL, 10);
> + errno = 0;
> + tmp_id = strtoul(value, NULL, 10);
> + if (errno != 0) {
> + goto done;
> + }
> + id->id = tmp_id;
> id->type = ID_TYPE_UID;
>
> ret = True;
> @@ -5709,7 +5750,12 @@ static NTSTATUS ldapsam_create_dom_group(struct pdb_methods *my_methods,
> return NT_STATUS_INTERNAL_DB_CORRUPTION;
> }
>
> + errno = 0;
> gid = strtoul(tmp, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert gidNumber\n");
> + return NT_STATUS_UNSUCCESSFUL;
> + }
>
> dn = smbldap_talloc_dn(tmp_ctx, priv2ld(ldap_state), entry);
> if (!dn) {
> @@ -5981,9 +6027,10 @@ static NTSTATUS ldapsam_change_groupmem(struct pdb_methods *my_methods,
> return NT_STATUS_INTERNAL_DB_CORRUPTION;
> }
>
> + errno = 0;
> user_gid = strtoul(gidstr, NULL, 10);
>
> - if (!sid_to_gid(&group_sid, &group_gid)) {
> + if (!sid_to_gid(&group_sid, &group_gid) || errno != 0) {
> DEBUG (0, ("ldapsam_change_groupmem: Unable to get group gid from SID!\n"));
> return NT_STATUS_UNSUCCESSFUL;
> }
> diff --git a/source3/passdb/pdb_tdb.c b/source3/passdb/pdb_tdb.c
> index 91735ff7084..963732bfaa6 100644
> --- a/source3/passdb/pdb_tdb.c
> +++ b/source3/passdb/pdb_tdb.c
> @@ -1182,7 +1182,11 @@ static int tdbsam_collect_rids(struct db_record *rec, void *private_data)
> return 0;
> }
>
> + errno = 0;
> rid = strtoul((char *)key.dptr+prefixlen, NULL, 16);
> + if (errno != 0) {
> + return 0;
> + }
>
> ADD_TO_LARGE_ARRAY(state, uint32_t, rid, &state->rids, &state->num_rids,
> &state->array_size);
> --
> 2.20.1
>
>
> From 3dcd271aabd180e78b649a6c5cb264c25eee5590 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 18:37:35 +0100
> Subject: [PATCH 5/7] winbindd: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/winbindd/idmap_ldap.c | 11 +++++++++--
> source3/winbindd/winbindd_lookuprids.c | 4 +++-
> source3/winbindd/winbindd_util.c | 18 +++++++++++++++++-
> 3 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/source3/winbindd/idmap_ldap.c b/source3/winbindd/idmap_ldap.c
> index 17cc7404f12..c73aac40bd7 100644
> --- a/source3/winbindd/idmap_ldap.c
> +++ b/source3/winbindd/idmap_ldap.c
> @@ -299,7 +299,12 @@ static NTSTATUS idmap_ldap_allocate_id_internal(struct idmap_domain *dom,
> goto done;
> }
>
> + errno = 0;
> xid->id = strtoul(id_str, NULL, 10);
> + if (errno != 0) {
> + ret = NT_STATUS_UNSUCCESSFUL;
> + goto done;
> + }
>
> /* make sure we still have room to grow */
>
> @@ -769,8 +774,9 @@ again:
> continue;
> }
>
> + errno = 0;
> id = strtoul(tmp, NULL, 10);
> - if (!idmap_unix_id_is_in_range(id, dom)) {
> + if (errno != 0 || !idmap_unix_id_is_in_range(id, dom)) {
> DEBUG(5, ("Requested id (%u) out of range (%u - %u). "
> "Filtered!\n", id,
> dom->low_id, dom->high_id));
> @@ -1005,8 +1011,9 @@ again:
> continue;
> }
>
> + errno = 0;
> id = strtoul(tmp, NULL, 10);
> - if (!idmap_unix_id_is_in_range(id, dom)) {
> + if (errno != 0 || !idmap_unix_id_is_in_range(id, dom)) {
> DEBUG(5, ("Requested id (%u) out of range (%u - %u). "
> "Filtered!\n", id,
> dom->low_id, dom->high_id));
> diff --git a/source3/winbindd/winbindd_lookuprids.c b/source3/winbindd/winbindd_lookuprids.c
> index 1e80b78a92e..b2982bf57b6 100644
> --- a/source3/winbindd/winbindd_lookuprids.c
> +++ b/source3/winbindd/winbindd_lookuprids.c
> @@ -182,8 +182,10 @@ static bool parse_ridlist(TALLOC_CTX *mem_ctx, char *ridstr,
>
> for (i=0; i<num_rids; i++) {
> char *q;
> +
> + errno = 0;
> rids[i] = strtoul(p, &q, 10);
> - if (*q != '\n') {
> + if (errno != 0 || *q != '\n') {
> DEBUG(0, ("Got invalid ridstr: %s\n", p));
> return false;
> }
> diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
> index d266eb3048e..4508d96dba9 100644
> --- a/source3/winbindd/winbindd_util.c
> +++ b/source3/winbindd/winbindd_util.c
> @@ -506,7 +506,12 @@ static void trustdom_list_done(struct tevent_req *req)
> break;
> }
>
> + errno = 0;
> trust_flags = (uint32_t)strtoul(q, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert trust_flags\n");
> + break;
> + }
>
> q = strtok(NULL, "\\");
> if (q == NULL) {
> @@ -514,7 +519,12 @@ static void trustdom_list_done(struct tevent_req *req)
> break;
> }
>
> + errno = 0;
> trust_type = (uint32_t)strtoul(q, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert trust_type\n");
> + break;
> + }
>
> q = strtok(NULL, "\n");
> if (q == NULL) {
> @@ -522,7 +532,12 @@ static void trustdom_list_done(struct tevent_req *req)
> break;
> }
>
> + errno = 0;
> trust_attribs = (uint32_t)strtoul(q, NULL, 10);
> + if (errno != 0) {
> + DBG_ERR("Failed to convert trust_attribs\n");
> + break;
> + }
>
> if (!within_forest) {
> trust_flags &= ~NETR_TRUST_FLAG_IN_FOREST;
> @@ -2156,8 +2171,9 @@ bool parse_xidlist(TALLOC_CTX *mem_ctx, const char *xidstr,
>
> p += 1;
>
> + errno = 0;
> id = strtoull(p, &endp, 10);
> - if ((id == ULLONG_MAX) && (errno == ERANGE)) {
> + if (errno != 0) {
> goto fail;
> }
> if (*endp != '\n') {
> --
> 2.20.1
>
>
> From 6caa31a6dccb2c007ccaca472140716db338546b Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 18:41:23 +0100
> Subject: [PATCH 6/7] modules: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/modules/vfs_preopen.c | 4 ++++
> source3/modules/vfs_snapper.c | 3 ++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/source3/modules/vfs_preopen.c b/source3/modules/vfs_preopen.c
> index 84b13556195..a87a7075ff3 100644
> --- a/source3/modules/vfs_preopen.c
> +++ b/source3/modules/vfs_preopen.c
> @@ -363,7 +363,11 @@ static bool preopen_parse_fname(const char *fname, unsigned long *pnum,
> return false;
> }
>
> + errno = 0;
> num = strtoul(p, (char **)&q, 10);
> + if (errno != 0) {
> + return false;
> + }
>
> if (num+1 < num) {
> /* overflow */
> diff --git a/source3/modules/vfs_snapper.c b/source3/modules/vfs_snapper.c
> index 443a940b17a..c641f0d0618 100644
> --- a/source3/modules/vfs_snapper.c
> +++ b/source3/modules/vfs_snapper.c
> @@ -1249,9 +1249,10 @@ static NTSTATUS snapper_snap_path_to_id(TALLOC_CTX *mem_ctx,
> return NT_STATUS_INVALID_PARAMETER;
> }
>
> + errno = 0;
> str_idx++;
> snap_id = strtoul(str_idx, &str_end, 10);
> - if (str_idx == str_end) {
> + if (str_idx == str_end || errno != 0) {
> talloc_free(path_dup);
> return NT_STATUS_INVALID_PARAMETER;
> }
> --
> 2.20.1
>
>
> From 565e7f9afc06da335bcb046b0836f25b1720d947 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Thu, 10 Jan 2019 18:48:08 +0100
> Subject: [PATCH 7/7] rpcclient: Fix string to integer conversion
>
> In order to detect an error during the string to integer
> conversion with strtoul/strtoull, the errno variable must
> be set to zero before the execution and checked after the
> conversion is performed.
>
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> ---
> source3/rpcclient/cmd_samr.c | 4 ++++
> source3/rpcclient/cmd_spoolss.c | 5 +++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/source3/rpcclient/cmd_samr.c b/source3/rpcclient/cmd_samr.c
> index d55d1648c91..7f7a6604682 100644
> --- a/source3/rpcclient/cmd_samr.c
> +++ b/source3/rpcclient/cmd_samr.c
> @@ -1412,7 +1412,11 @@ static NTSTATUS cmd_samr_delete_alias(struct rpc_pipe_client *cli,
> return NT_STATUS_OK;
> }
>
> + errno = 0;
> alias_rid = strtoul(argv[2], NULL, 10);
> + if (errno != 0) {
> + return NT_STATUS_INVALID_PARAMETER;
> + }
>
> /* Open SAMR handle */
>
> diff --git a/source3/rpcclient/cmd_spoolss.c b/source3/rpcclient/cmd_spoolss.c
> index 8d330afdeb0..3475c391543 100644
> --- a/source3/rpcclient/cmd_spoolss.c
> +++ b/source3/rpcclient/cmd_spoolss.c
> @@ -2707,7 +2707,12 @@ static WERROR cmd_spoolss_setprinterdata(struct rpc_pipe_client *cli,
> W_ERROR_HAVE_NO_MEMORY(data.string);
> break;
> case REG_DWORD:
> + errno = 0;
> data.value = strtoul(argv[4], NULL, 10);
> + if (errno != 0) {
> + result = WERR_INVALID_PARAMETER;
> + goto done;
> + }
> break;
> case REG_BINARY:
> data.binary = strhex_to_data_blob(mem_ctx, argv[4]);
> --
> 2.20.1
>
More information about the samba-technical
mailing list