#47835 Coverity: 12687..12692
Closed: wontfix None Opened 9 years ago by nhosoi.

12692: Use of untrusted string value
12687..12691: Unbounded source buffer


git patch file (master) -- 12692 - Use of untrusted string value
0001-Ticket-47835-Coverity-12687.12692.patch

Reviewed by Rich (Thank you!!)

Pushed to master:
b6b7199..43c6ff2 master -> master
commit 43c6ff2
commit 0a546bc
commit 48f2ea0
commit 162604a
commit f25c7f1
commit 8dc3806

I would like to ask what is wrong with calling strdup. It should be safe.
I saw many replacement of strdup in patches. In my opinion, it is a false positive.

For example:
{{{
diff --git a/ldap/servers/slapd/tools/dbscan.c b/ldap/servers/slapd/tools/dbscan.c
index 023fade..bbfcd0e 100644
--- a/ldap/servers/slapd/tools/dbscan.c
+++ b/ldap/servers/slapd/tools/dbscan.c
@@ -1077,16 +1077,17 @@ is_changelog(char *filename)

static void usage(char argv0)
{
- char
copy = strdup(argv0);
+ long arg_max = sysconf(_SC_ARG_MAX);
+ char copy = strndup(argv0, arg_max);
}}}
Variable ''argv0'' has type ''char
''. So it is nul terminated string.

The manual page says:
{{{
The strdup() function returns a pointer to a new string which is a
duplicate of the string s. Memory for the new string is obtained with
malloc(3), and can be freed with free(3).
}}}

I cannot see a reason why there should be any problem. Could you explain me it? I would like to learn something new.

Yes, it could be false positive. I'm just worried the case when the given string is corrupted/tainted (e.g., no NULL terminated).

We had this code for years and have not heard of any security issue. But we thought it was a good practice to set the maximum length to the string to duplicate not to overflow anything... At least, you see no harm on that, don't you?

I'm not sure if a string passed from an environment variable, or from a command line argument, can really be non-NULL terminated. We should do some research on that, and also possibly find out what Coverity users usually do to fix such issues.

I cannot say it was a complete research :p, but it was based upon the search result on the coverity error and fix... I don't mind undoing the changes and just mark "false positive" on coverity if that's better.

One line fix:
13ec1d6..6e175e3 master -> master
commit 6e175e3
Description: arg_max has to be always set. But introduced by
commit 0a546bc

It turned out these defects are false positive.
Reverting...
commit 43c6ff2
commit 0a546bc
commit 48f2ea0
commit 162604a
commit f25c7f1
commit 8dc3806
commit 6e175e3

Reverted:
daf4b42..3a66ec7 master -> master
{{{
commit 3a66ec7
This reverts commit 43c6ff2.
commit dc4527b
This reverts commit 0a546bc.
commit f501430
This reverts commit 48f2ea0.
commit 5065496
This reverts commit 162604a.
commit cef8209
This reverts commit f25c7f1.
commit 8247976
This reverts commit 8dc3806.
commit 4b66c03
This reverts commit 6e175e3.
}}}

Metadata Update from @nhosoi:
- Issue set to the milestone: 1.3.3 backlog

7 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1166

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Invalid)

3 years ago

Login to comment on this ticket.

Metadata