full_name_format is documented as a printf(3) compatible format string, and the util.c implementation passes that string to snprintf after some sneaky hacks to make it actually work ...
It's not good practice to pass user input (even admin input) to printf directly. Doing so leads to crashes and abort()'s when the format string isn't just right. This probably isn't exploitable, but as usual it's hard to be sure :)
Fedora and others check for sorta thing with -Wformat-security flags.
Unlike regular expressions, printf format strings aren't meant for user provided input. There are probably better choices for the full_name_format syntax. I think any work on that should be separate from this patch.
Anyhow, here's a patch which aims to make the full_name_format printf handling both correct and safe.
This patch includes safe-printf.[ch] from the realmd project, which are written with just standard libc deps, and uses it for formatting fully qualified names. [1]
In addition there were various corner cases and assumptions about the format strings, such as the exact syntax of the args, the length of the output, etc... which I've fixed in this patch.
Tests are added and updated.
I'll be happy to split the patch into two, if desired. One which adds safe-printf.[ch] + tests, and the second which fixes the fully qualified names.
I believe that this patch also simplifies fqname handling code.
Cheers,
Stef
[1] realmd currently uses full_name_format to build it's LoginFormats property. Once sssd is on the system DBus bus, realmd might be able to ask sssd for this directly...
On 07.01.2014 14:07, Stef Walter wrote:
Anyhow, here's a patch which aims to make the full_name_format printf handling both correct and safe.
...
I'll be happy to split the patch into two, if desired. One which adds safe-printf.[ch] + tests, and the second which fixes the fully qualified names.
Now split for easier review.
Stef
On 07.01.2014 14:57, Stef Walter wrote:
On 07.01.2014 14:07, Stef Walter wrote:
Anyhow, here's a patch which aims to make the full_name_format printf handling both correct and safe.
...
I'll be happy to split the patch into two, if desired. One which adds safe-printf.[ch] + tests, and the second which fixes the fully qualified names.
Now split for easier review.
Stephen suggested I put the safe-printf part of this into its own library. Not my first choice, but here we go. The first patch is for ding-libs, and the second for sssd.
Cheers,
Stef
A few notes:
1. style although we allow things like if (condition) statement; I do not think we accpet for or while loops w/o braces
2. the functions are incomplete and lack documentation that states so in particular the only accepted format type is 's' (string). I think we want a complete replacement for [a|sn]printf if you want to put it in ding-libs. If you do not want to make the general puprpose version then I propose you contribute this only as a private function for sssd, but please add documentation in the header file about what formatting is supported.
3. can you change from 'callback' to 'copy_fn' ?
On the code, looks mostly ok except the incompleteness of the printf function, however:
On Tue, 2014-01-07 at 16:07 +0100, Stef Walter wrote:
+static void +snprintf_callback (void *data,
const char *piece,
size_t length)
+{
- struct sprintf_ctx *cx = data;
- /* Don't copy if too much data */
- if (cx->length > cx->alloc)
length = 0;
- else if (cx->length + length > cx->alloc)
length = cx->alloc - cx->length;
- else
length = length;
The last one branch is unnecessary and a bit useless :-)
Simo.
On 07.01.2014 19:55, Simo Sorce wrote:
A few notes:
- style
although we allow things like if (condition) statement; I do not think we accpet for or while loops w/o braces
Nod.
- the functions are incomplete and lack documentation that states so
in particular the only accepted format type is 's' (string). I think we want a complete replacement for [a|sn]printf if you want to put it in ding-libs. If you do not want to make the general puprpose version then I propose you contribute this only as a private function for sssd, but please add documentation in the header file about what formatting is supported.
The reason that the printf positional arguments are so brittle (one must consume them all) is exactly because arguments of different sizes are accepted in the varargs. So obviously the positional code cannot know the relevant vararg if some are not consumed.
Having the only accepted format type be 's' follows from that. Otherwise it would be difficult to have a user-provided format string.
But yes, general purpose is not the goal. Fixing sssd to be robust and correct is the goal. I'd much rather not put this sorta thing in general purpose library like ding-libs.
- can you change from 'callback' to 'copy_fn' ?
Sure.
On the code, looks mostly ok except the incompleteness of the printf function, however:
On Tue, 2014-01-07 at 16:07 +0100, Stef Walter wrote:
+static void +snprintf_callback (void *data,
const char *piece,
size_t length)
+{
- struct sprintf_ctx *cx = data;
- /* Don't copy if too much data */
- if (cx->length > cx->alloc)
length = 0;
- else if (cx->length + length > cx->alloc)
length = cx->alloc - cx->length;
- else
length = length;
The last one branch is unnecessary and a bit useless :-)
Oops. Refactoring strikes again...
Stef
On Tue, 2014-01-07 at 20:20 +0100, Stef Walter wrote:
On 07.01.2014 19:55, Simo Sorce wrote:
A few notes:
- style
although we allow things like if (condition) statement; I do not think we accpet for or while loops w/o braces
Nod.
- the functions are incomplete and lack documentation that states so
in particular the only accepted format type is 's' (string). I think we want a complete replacement for [a|sn]printf if you want to put it in ding-libs. If you do not want to make the general puprpose version then I propose you contribute this only as a private function for sssd, but please add documentation in the header file about what formatting is supported.
The reason that the printf positional arguments are so brittle (one must consume them all) is exactly because arguments of different sizes are accepted in the varargs. So obviously the positional code cannot know the relevant vararg if some are not consumed.
Having the only accepted format type be 's' follows from that. Otherwise it would be difficult to have a user-provided format string.
But yes, general purpose is not the goal. Fixing sssd to be robust and correct is the goal. I'd much rather not put this sorta thing in general purpose library like ding-libs.
Ok fine, makes sense once explained (need this explanation in the docs/headers), but then use a different name.
If I see safe_snprintf, I assume the full format capabilities of snprintf, the name in this case is misleading.
That said I am not sure what's the best name you could use... maybe strextend ? and the astrextend variant for allocated ones ?
- can you change from 'callback' to 'copy_fn' ?
Sure.
On the code, looks mostly ok except the incompleteness of the printf function, however:
On Tue, 2014-01-07 at 16:07 +0100, Stef Walter wrote:
+static void +snprintf_callback (void *data,
const char *piece,
size_t length)
+{
- struct sprintf_ctx *cx = data;
- /* Don't copy if too much data */
- if (cx->length > cx->alloc)
length = 0;
- else if (cx->length + length > cx->alloc)
length = cx->alloc - cx->length;
- else
length = length;
The last one branch is unnecessary and a bit useless :-)
Oops. Refactoring strikes again...
:-)
Simo.
On 07.01.2014 20:34, Simo Sorce wrote:
Ok fine, makes sense once explained (need this explanation in the docs/headers), but then use a different name.
If I see safe_snprintf, I assume the full format capabilities of snprintf, the name in this case is misleading.
That said I am not sure what's the best name you could use... maybe strextend ? and the astrextend variant for allocated ones ?
I've called it safe_str_snprintf (and so on). Hopefully that's clear enough. It is processing the familiar printf syntax, so it makes sense to have that somewhere.
Updated header documentation as requested, and made other fixes from your earlier review.
Cheers,
Stef
On Tue, 2014-01-07 at 21:31 +0100, Stef Walter wrote:
On 07.01.2014 20:34, Simo Sorce wrote:
Ok fine, makes sense once explained (need this explanation in the docs/headers), but then use a different name.
If I see safe_snprintf, I assume the full format capabilities of snprintf, the name in this case is misleading.
That said I am not sure what's the best name you could use... maybe strextend ? and the astrextend variant for allocated ones ?
I've called it safe_str_snprintf (and so on). Hopefully that's clear enough. It is processing the familiar printf syntax, so it makes sense to have that somewhere.
Personally the problem is in using 'printf' in there (both in function name and file names), because although similar it doesn't support all of printf semantics, and it can't, it will confuse peole. I do not think a _str_ prefix really makes a difference. The other issue is that you may later find people "extending" safe_snprintf() to "support the other format arguments" and we are back to an "unsafe" function.
Maybe: safe_format_string() ?
Updated header documentation as requested, and made other fixes from your earlier review.
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
The rest looks good to me.
Simo.
On 07.01.2014 22:21, Simo Sorce wrote:
On Tue, 2014-01-07 at 21:31 +0100, Stef Walter wrote:
On 07.01.2014 20:34, Simo Sorce wrote:
Ok fine, makes sense once explained (need this explanation in the docs/headers), but then use a different name.
If I see safe_snprintf, I assume the full format capabilities of snprintf, the name in this case is misleading.
That said I am not sure what's the best name you could use... maybe strextend ? and the astrextend variant for allocated ones ?
I've called it safe_str_snprintf (and so on). Hopefully that's clear enough. It is processing the familiar printf syntax, so it makes sense to have that somewhere.
Personally the problem is in using 'printf' in there (both in function name and file names), because although similar it doesn't support all of printf semantics, and it can't, it will confuse peole. I do not think a _str_ prefix really makes a difference. The other issue is that you may later find people "extending" safe_snprintf() to "support the other format arguments" and we are back to an "unsafe" function.
Maybe: safe_format_string() ?
Sure ... done.
Updated header documentation as requested, and made other fixes from your earlier review.
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Stef
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
On Tue, 2014-01-07 at 21:31 +0100, Stef Walter wrote:
On 07.01.2014 20:34, Simo Sorce wrote:
Ok fine, makes sense once explained (need this explanation in the docs/headers), but then use a different name.
If I see safe_snprintf, I assume the full format capabilities of snprintf, the name in this case is misleading.
That said I am not sure what's the best name you could use... maybe strextend ? and the astrextend variant for allocated ones ?
I've called it safe_str_snprintf (and so on). Hopefully that's clear enough. It is processing the familiar printf syntax, so it makes sense to have that somewhere.
Personally the problem is in using 'printf' in there (both in function name and file names), because although similar it doesn't support all of printf semantics, and it can't, it will confuse peole. I do not think a _str_ prefix really makes a difference. The other issue is that you may later find people "extending" safe_snprintf() to "support the other format arguments" and we are back to an "unsafe" function.
Maybe: safe_format_string() ?
Sure ... done.
Thanks.
Updated header documentation as requested, and made other fixes from your earlier review.
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
Simo.
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
Cheers,
Stef
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
On 08.01.2014 23:27, Jakub Hrozek wrote:
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem problems in patch 0002. fq_len and nlen are now int.
Stef
On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
On 08.01.2014 23:27, Jakub Hrozek wrote:
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
Sorry I forgot another, I think you should either set errno on errors, or return an errno_t instead of -1. Just returning -1 for all errors is a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem problems in patch 0002. fq_len and nlen are now int.
Stef
The patches look good to me and all the testing I did (various formats of FQDN including invalid, no FQDN, groups with mixed-domain membership) went fine.
ACK.
On 10.01.2014 12:11, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
On 08.01.2014 23:27, Jakub Hrozek wrote:
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote: > Sorry I forgot another, I think you should either set errno on errors, > or return an errno_t instead of -1. Just returning -1 for all errors is > a poor interface.
It's the same interface as the functions being replaced: snprintf and asprintf. We return the full format length, or -1 on failure. snprintf and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem problems in patch 0002. fq_len and nlen are now int.
Stef
The patches look good to me and all the testing I did (various formats of FQDN including invalid, no FQDN, groups with mixed-domain membership) went fine.
Updated version which no longer conflicts with the .gitignore patch.
Stef
On Fri, Jan 10, 2014 at 02:56:42PM +0100, Stef Walter wrote:
On 10.01.2014 12:11, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
On 08.01.2014 23:27, Jakub Hrozek wrote:
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote:
On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote: > On 07.01.2014 22:21, Simo Sorce wrote: >> Sorry I forgot another, I think you should either set errno on errors, >> or return an errno_t instead of -1. Just returning -1 for all errors is >> a poor interface. > > It's the same interface as the functions being replaced: snprintf and > asprintf. We return the full format length, or -1 on failure. snprintf > and friends don't set errno. See man printf(3). > > But I guess now that we're diverging from the interface provided by > snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it > makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM happened, but I see you removed the alloc version ... I guess in this case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem problems in patch 0002. fq_len and nlen are now int.
Stef
The patches look good to me and all the testing I did (various formats of FQDN including invalid, no FQDN, groups with mixed-domain membership) went fine.
Updated version which no longer conflicts with the .gitignore patch.
Stef
ACK again, I'm just putting the patch through Coverity before pushing.
On Fri, Jan 10, 2014 at 03:19:07PM +0100, Jakub Hrozek wrote:
On Fri, Jan 10, 2014 at 02:56:42PM +0100, Stef Walter wrote:
On 10.01.2014 12:11, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
On 08.01.2014 23:27, Jakub Hrozek wrote:
On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
On 08.01.2014 17:59, Simo Sorce wrote: > On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote: >> On 07.01.2014 22:21, Simo Sorce wrote: >>> Sorry I forgot another, I think you should either set errno on errors, >>> or return an errno_t instead of -1. Just returning -1 for all errors is >>> a poor interface. >> >> It's the same interface as the functions being replaced: snprintf and >> asprintf. We return the full format length, or -1 on failure. snprintf >> and friends don't set errno. See man printf(3). >> >> But I guess now that we're diverging from the interface provided by >> snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it >> makes you happy. > > Well I generally prefer to know if the format was wrong or ENOMEM > happened, but I see you removed the alloc version ... I guess in this > case I am not too picky, only format errors can happen now.
I figured that neither neither realmd or sssd was using the asprintf variant (both used other allocation schemes) I might as well remove it.
I agree it's better to not have code that's not used in the repo at all. Such code can easily become subject to bitrot (it wouldn't be the case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem problems in patch 0002. fq_len and nlen are now int.
Stef
The patches look good to me and all the testing I did (various formats of FQDN including invalid, no FQDN, groups with mixed-domain membership) went fine.
Updated version which no longer conflicts with the .gitignore patch.
Stef
ACK again, I'm just putting the patch through Coverity before pushing.
Coverity finally finished and did not discover any new errors.
Pushed to master!
sssd-devel@lists.fedorahosted.org