--
-----------------------------------------------------------------------------
| Phil Howard KA9WGN | http://linuxhomepage.com/ http://ham.org/ |
| (first name) at ipal.net | http://phil.ipal.org/ http://ka9wgn.ham.org/ |
-----------------------------------------------------------------------------
> Is there a way I can verify file ownership of a dynamic library I will be
> opening with dlopen()? If the program that is opening it is running suid,
> especially suid root, I want to be sure the library is owned by the suid'd
> effective user. In particular, I want to be sure the library links cannot
> be switched around between stat() and dlopen(). If this were just open(),
> I could do fstat() on the descriptor to be really sure. Something like
> fdopen() but for a library ... like fdlopen() ... would be nice. But I
> don't see such a thing around.
I'm not aware of such a thing either. One possible solution which
comes to my mind is to intercept calls to open() and do the
verification there, i.e. call the real open(), fstat(), and return the
fd only if the file has the right owner, otherwise closing it and
returning -1.
--
M�ns Rullg�rd
ma...@mansr.com
The following code worked for me on OS X, Linux, and OpenBSD without
modification, excepting the original library path. Obvious caveats would be
inability to work in a chroot jail without /dev/fd visibility, and at
least on OpenBSD /dev/fd entries are static device files so it wouldn't work
if the fd was too high--on a vanilla OpenBSD install over 64.
#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <dlfcn.h>
#include <err.h>
int main(void) {
char path[256] = "/usr/local/lib/libyaml.so.0.0";
char name[64] = "yaml_get_version_string";
int fd;
void *lib;
const char *(*getver)(void);
if (-1 == (fd = open(path, O_RDONLY)))
err(EXIT_FAILURE, "%s", path);
snprintf(path, sizeof path, "/dev/fd/%d", fd);
if (!(lib = dlopen(path, RTLD_NOW)))
errx(EXIT_FAILURE, "%s: %s", path, dlerror());
if (!(getver = dlsym(lib, name)))
errx(EXIT_FAILURE, "%s: %s", name, dlerror());
printf("%s\n", getver());
return 0;
}
> Is there a way I can verify file ownership of a dynamic library I will be
> opening with dlopen()? If the program that is opening it is running suid,
> especially suid root, I want to be sure the library is owned by the suid'd
> effective user. In particular, I want to be sure the library links cannot
> be switched around between stat() and dlopen(). If this were just open(),
> I could do fstat() on the descriptor to be really sure. Something like
> fdopen() but for a library ... like fdlopen() ... would be nice. But I
> don't see such a thing around.
I think it's likely a better idea to check the ownership and
permissions of the directory the library is in before calling
'dlopen'. The ownership of the file doesn't really say much about
whether it's safe to use for the purpose it's being used anyway.
DS
He needs to check every path element that leads to the library, because
either one could be a symlink.
And the check must be aware of the local system's rules for permissions. For
example, just checking the file mode may not be enough if there are ACLs.
He also needs to check that the library has no runtime dependency towards
untrusted paths. For example, with modern automagic tools, if someone
compiles with --prefix=/tmp/install and then moves the installation (he'd
rather use DESTDIR, of course), he can get RPATH=/tmp/install/lib in some of
his libraries.
Even if the library is in a completely trusted path and does not have
structural dependencies towards untrusted paths, it could be a completely
unrelated library unsuited to run in a SUID environment. Some libraries, for
example, parse some specific environment variable in their _init function.
> #include <stdlib.h>
> #include <stdio.h>
>
> #include <fcntl.h>
>
> #include <dlfcn.h>
>
> #include <err.h>
>
> int main(void) {
> char path[256] = "/usr/local/lib/libyaml.so.0.0";
> char name[64] = "yaml_get_version_string";
> int fd;
> void *lib;
> const char *(*getver)(void);
>
> if (-1 == (fd = open(path, O_RDONLY)))
> err(EXIT_FAILURE, "%s", path);
>
> snprintf(path, sizeof path, "/dev/fd/%d", fd);
>
> if (!(lib = dlopen(path, RTLD_NOW)))
> errx(EXIT_FAILURE, "%s: %s", path, dlerror());
>
> if (!(getver = dlsym(lib, name)))
I think it might be better (for some definition of "better") to write this
as
if (!(*(void **)&getver = dlsym(lib, name)))
See [0], [1], and [2].
> errx(EXIT_FAILURE, "%s: %s", name, dlerror());
>
> printf("%s\n", getver());
>
> return 0;
> }
>
Cheers,
lacos
[0] http://www.opengroup.org/onlinepubs/9699919799/functions/dlsym.html#tag_16_96_06
[1] http://www.opengroup.org/onlinepubs/9699919799/functions/dlsym.html#tag_16_96_08
[2] http://www.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_12_03
> David Schwartz wrote in message
> <f37d8057-9e6b-4e9a...@k25g2000prh.googlegroups.com>:
>> I think it's likely a better idea to check the ownership and
>> permissions of the directory the library is in before calling 'dlopen'.
>
> He needs to check every path element that leads to the library, because
> either one could be a symlink.
>
> And the check must be aware of the local system's rules for permissions.
> For example, just checking the file mode may not be enough if there are
> ACLs.
Once the full pathname has been tokenized, the way to do that might be
something like this. (Note - I didn't even try to compile this. It's here
only to invite criticism and/or better ideas.)
int
safeopen(const char * const * elem, size_t nelem)
{
if (0u < nelem) {
int fd;
size_t cur;
fd = open("/", O_SEARCH | O_DIRECTORY | O_NOFOLLOW);
cur = 0u;
loop:
if (-1 != fd) {
struct stat s;
if (0 == fstat(fd, &s)) {
if (cur == nelem) {
if (S_ISREG(s.st_mode) && /* other checks on the executable */) {
return fd;
}
}
else {
if (/* checks on the directory */) {
int tmp;
tmp = fd;
fd = openat(fd, elem[cur], O_NOFOLLOW
| (cur < nelem - 1u ? O_SEARCH | O_DIRECTORY : O_EXEC));
if (0 == close(tmp)) {
++cur;
goto loop;
}
}
}
}
if (-1 != fd) {
(void)close(fd);
}
}
}
return -1;
}
The returned int (if not -1) could be used with fexcve(). Unfortunately
(or not), there is no "dlfdopen()".
The code could hang indefinitely if the last component identified a FIFO.
I'd OR O_NONBLOCK with O_EXEC, if not for O_NONBLOCK being unspecified for
regular files. (I can't see anything in the openat() spec that would
require O_EXEC to fail on a FIFO (and immediately).)
> He also needs to check that the library has no runtime dependency
> towards untrusted paths. For example, with modern automagic tools, if
> someone compiles with --prefix=/tmp/install and then moves the
> installation (he'd rather use DESTDIR, of course), he can get
> RPATH=/tmp/install/lib in some of his libraries.
>
> Even if the library is in a completely trusted path and does not have
> structural dependencies towards untrusted paths, it could be a
> completely unrelated library unsuited to run in a SUID environment. Some
> libraries, for example, parse some specific environment variable in
> their _init function.
We've been at the same spot in the near past: a process asking the kernel
(or whomever) for protection from code it executes.
Cheers,
lacos
Mea culpa.
Recent versions of GCC enforce strict aliasing so rigorously that I
instinctively refrain from such tricks. Indeed, GCC 4.5 complains at -O2.
Being forced to type pun to get a valid lvalue for such common,
unobjectionable usage is ridiculous. The standard should make conversions
from void pointers to function pointers unspecified so GCC, et al wouldn't
have to emit a diagnostic when building in a POSIX environment. But there
are no changes along these lines in C1X AFAICT.
> Ersek, Laszlo <la...@caesar.elte.hu> wrote:
>> On Tue, 4 May 2010, William Ahern wrote:
> <snip>
>> > if (!(getver = dlsym(lib, name)))
>>
>> I think it might be better (for some definition of "better") to write this
>> as
>>
>> if (!(*(void **)&getver = dlsym(lib, name)))
>
> Mea culpa.
>
> Recent versions of GCC enforce strict aliasing so rigorously that I
> instinctively refrain from such tricks. Indeed, GCC 4.5 complains at -O2.
It is right to complain. That code would not work on an
implementation where pointer-to-void and pointer-to-function have
different representations.
The dlsym() example in POSIX that uses similar code is a bug in POSIX,
and is being corrected by this interpretation:
http://austingroupbugs.net/view.php?id=74#c205
which (among many other things) changes the relevant line in the
example from
*(void **)(&fptr) = dlsym(handle, "my_function");
back to
fptr = (int (*)(int))dlsym(handle, "my_function");
(which is how it was in SUSv2). This might also produce a warning
in some compilers, but implementations are required to accept it
and "do the right thing".
--
Geoff Clare <net...@gclare.org.uk>
> It is right to complain. That code would not work on an
> implementation where pointer-to-void and pointer-to-function have
> different representations.
Except on POSIX systems they cannot--2.12.3--and this sort of type-punning
is the only way to get around the C standard.
> The dlsym() example in POSIX that uses similar code is a bug in POSIX,
> and is being corrected by this interpretation:
> http://austingroupbugs.net/view.php?id=74#c205
> which (among many other things) changes the relevant line in the
> example from
> *(void **)(&fptr) = dlsym(handle, "my_function");
> back to
> fptr = (int (*)(int))dlsym(handle, "my_function");
> (which is how it was in SUSv2). This might also produce a warning
> in some compilers, but implementations are required to accept it
> and "do the right thing".
Required by which standard? All conversions from a void pointer to a
function pointer are undefined behavior in C, and all implementations are
required to emit a diagnostic, and allowed to bail. If a function pointer is
wider than a void pointer, for instance, how does an explicit conversion
change anything? That's rhetorical, because it in fact doesn't, as far as
the C standard is concerned.
The problem with the C standard should be fixed. Even if POSIX/SUSv2
mandates certain behavior for an explicit conversion from void to function
pointer, the C standard requires at least a diagnostic, and on GCC and
others you can't use the highest level of warning and still build the file.
POSIX's use of type-punning, and the 2.12.3 requirement that void pointers
and functions pointers have the same representation, are ugly, yet the only
way to make a function like dlsym() available to a strictly conforming C
program.
I don't understand why the Austin Group would entertain returning to the
casting form. If the intention is to be able to drop the 2.12.3 requirement,
then they've just shot themselves in the foot.
Can you please cite the relevant parts of the C standard that require
a diagnostic for that case? When this was most recently discussed on
the austin-group mailing list, no one could find such a requirement,
while there were agreed to be issues for the *(void **)&fptr from. To
pick an apropos message from the archive:
...though you should examine the entire thread for context. Visit
http://www.opengroup.org/austin/mailarchives/ag/
and do a subject search for "0000074".
Philip Guenther
> On May 6, 8:28�am, William Ahern <will...@wilbur.25thandClement.com>
> wrote:
>> [snip] All conversions from a void pointer to a function pointer are
>> undefined behavior in C, and all implementations are required to emit a
>> diagnostic, and allowed to bail.
> Can you please cite the relevant parts of the C standard that require a
> diagnostic for that case?
I looked at C99 5.1.1.3 "Diagnostics" and 6.3.2.3 "Pointers". While
converting a pointer-to-void to a pointer-to-function is certainly
undefined behavior "by the omission of any explicit definition of
behavior" (C99 4. "Conformance" p2), I must admit that 6.3.2.3 doesn't
contain an explicit "Constraints" part -- like many other sections do
(most notably, the descriptions of operators). Therefore, 5.1.1.3 doesn't
seem to apply.
----v----
5.1.1.3 Diagnostics
1 A conforming implementation shall produce at least one diagnostic message
(identified in an implementation-defined manner) if a preprocessing
translation unit or translation unit contains a violation of any syntax
rule or constraint, even if the behavior is also explicitly specified as
undefined or implementation-defined. Diagnostic messages need not be
produced in other circumstances. [...]
----^----
In short, the explicit conversion (the cast operation) in question is
"only" undefined behavior, but (so it seems) not a syntax rule or
constraint violation, thus a diagnostic message need not be produced.
The current wording of SUSv4 XSH 2.12.3 "Pointer Types" defines the
conversion. It likely permits the type-punned pointer access too, by
fixing the representation. (Though I'm not sure if an additional note on
alignment is not missing.)
Cheers,
lacos
Perhaps. But my take on it was that since the conversion is undefined, what
exactly are you assigning? The pointer types in an assignment must be
compatible, otherwise it's an express constraint violation. See 6.5.16.1,
C99:TC3 draft n1256. If you can't write an expression which is assignable
under the constraints of 6.5.16.1, then presumably that requires a
diagnostic.
6.5.4, concerning the cast operator, on it's face seems to support the idea
that a cast is a declaration of the type, end of story, and any cast to a
proper type would be sufficient to meet the contraints of 6.5.16.1, no
matter whether the conversion is defined. And I agree this is the intuitive
interpretation.
But section 6.3 on conversions says that there's nothing special about a
cast. Casts can't obtain by _explicit_ conversion what isn't fundamentally
convertible; they can only obtain what _implicit_ conversion cannot, and of
course there are no implicit conversions between non-void pointers. Without
a conversion the type of the expression cannot change, and so you cannot
meet the 6.5.16.1 contraints.
In support of this latter interpretation, I should note that GCC in pedantic
mode does emit a diagnostic. At least according to my local man page, the
sole effect of `-pedantic' is to "issue all the warnings demanded by strict
ISO C". Though, the diagnostic description concerns the conversion, not the
assignment. But if we're concerned with _practical_ interpretations of the
standard, rather than _intuitive_, then GCC is persuasive authority.
> The current wording of SUSv4 XSH 2.12.3 "Pointer Types" defines the
> conversion. It likely permits the type-punned pointer access too, by
> fixing the representation. (Though I'm not sure if an additional note on
> alignment is not missing.)
True. There doesn't seem to be anything else which guarantees that a void
pointer doesn't have stricter alignment than a function pointer.
> Ersek, Laszlo <la...@caesar.elte.hu> wrote:
>> On Thu, 6 May 2010, guen...@gmail.com wrote:
>>> On May 6, 8:28?am, William Ahern <will...@wilbur.25thandClement.com>
>>> wrote:
>>
>>>> [snip] All conversions from a void pointer to a function pointer are
>>>> undefined behavior in C, and all implementations are required to emit a
>>>> diagnostic, and allowed to bail.
>>
>>> Can you please cite the relevant parts of the C standard that require a
>>> diagnostic for that case?
>>
>> I looked at C99 5.1.1.3 "Diagnostics" and 6.3.2.3 "Pointers". While
>> converting a pointer-to-void to a pointer-to-function is certainly
>> undefined behavior "by the omission of any explicit definition of
>> behavior" (C99 4. "Conformance" p2), I must admit that 6.3.2.3 doesn't
>> contain an explicit "Constraints" part -- like many other sections do
>> (most notably, the descriptions of operators). Therefore, 5.1.1.3 doesn't
>> seem to apply.
> <snip>
>> In short, the explicit conversion (the cast operation) in question is
>> "only" undefined behavior, but (so it seems) not a syntax rule or
>> constraint violation, thus a diagnostic message need not be produced.
>
> Perhaps. But my take on it was that since the conversion is undefined,
I've probably lost the thread of your reasoning, but I believe the point
is, the POSIX (SUSv4) XSH 2.12.3 "Pointer Types" section defines both the
representation and the conversion (separately). Thus if you take the C99
standard and fill in this single (intentional) white spot, and change
nothing else, the resultant specification "fixes" the *only* problem with
the expressions in question.
> what exactly are you assigning? The pointer types in an assignment must
> be compatible, otherwise it's an express constraint violation.
The
*(void **)&funptr = dlsym(...)
assignment has no problems with type compatibility (both sides are of type
pointer-to-void), it has problems with representation and alignment (even
before funptr is accessed in its own right). They are (or at least,
representation is) fixed by POSIX.
The
funptr = (funptr_type)dlsym(...)
assignment has no problem with type compatibility (both sides have type
funptr_type), it has problems with undefined conversion. That's also fixed
(defined) by POSIX.
I believe neither expression violates the constrains of simple assignment.
[snip]
Cheers,
lacos
> I believe neither expression violates the constrains of simple assignment.
My thread of reasoning is that simple assignment of pointers requires
compatible _types_, without which a diagnostic is required. In order to get
compatible types you need to have a conversion. There's no implicit
conversion allowed here, thus you need an explicit conversion--a cast. But a
cast can't convert what isn't covertible. There can be no conversion from a
void or object pointer to a function pointer. (Note that 6.3.2.3p7 allows
conversion between object pointers, so a typical object pointer cast is
making a well-defined conversion.) Therefore, there can't be a compatible
type to the right operand of the assignment no matter how emphatic the cast,
and the assignment constraint can be met.
Query: does the following require a diagnostic?
struct s { int a; } s;
int i;
i = (int)s; /* pretty please! */
I propose that the above is indistinguishable in this context from
attempting to cast a void pointer to a function pointer. I'm not sure it
improves my argument about requiring a diagnostic, but it does a better job
of showing what kind of distinctions I'm making.
[snip]
> There can be no conversion from a void or object pointer to a function
> pointer.
AFAICT the ISO C standard doesn't prohibit this for any and all derivate
standards. It simply doesn't define this kind of conversion.
Hereby I'll appeal to authority: see [0].
(For the "authority" part, see [1], [2], [3].)
> Query: does the following require a diagnostic?
>
> struct s { int a; } s;
> int i;
>
> i = (int)s; /* pretty please! */
I won't try to answer that, but I think defining something in a derivative
standard that was left undefined by the ISO C standard is different than
changing an "inherited" constraint. This seems to be supported by the
wording of the CX margin code [4] ("any conflict is unintentional").
Cheers,
lacos
[0] http://groups.google.com/group/comp.lang.c.moderated/msg/9424a7526e343c52
Message-ID: <clcm-2009...@plethora.net>
[1] http://www.opengroup.org/onlinepubs/9699919799/frontmatter/participants.html
[2] http://groups.google.com/group/comp.lang.c.moderated/msg/e9a5008c16c28f49
Message-ID: <clcm-2009...@plethora.net>
[3] http://groups.google.com/group/comp.lang.c.moderated/msg/159aae9d761d79ef
Message-ID: <clcm-2010...@plethora.net>
[4] http://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap01.html#tag_01_07_01
The fact that the committee is vascillating between type-punning and
cast-to-function-pointer suggests that there's confusion. What's most
clearly problematic as far as standards conformance goes, is the suggestion
in the comments to the defect report that they could drop the 2.12.3 pointer
equivalence clause. That's necessary no matter what method.
BTW, it just occurred to me that the following is allowable. I can't believe
that I didn't catch it earlier--6.3.2.3, para. 5 and 6.
const char *(*getver)(void);
getver = ((const char *)(*)(void))(intptr_t)dlsym(lib, name);
It of course depends on representation.
> What's most clearly problematic as far as standards conformance goes, is
> the suggestion in the comments to the defect report that they could drop
> the 2.12.3 pointer equivalence clause. That's necessary no matter what
> method.
(I suppose you're implying
<http://austingroupbugs.net/view.php?id=74#c156>.)
Dropping 2.12.3 would obliterate a generic requirement. dlsym() could
still be required to return appropriate values -- that would define a more
"narrow" behavior than 2.12.3 does now, ie. a proper subset of 2.2.13.
Thus it would likely not clash with anything in ISO C, just as 2.12.3
doesn't.
> BTW, it just occurred to me that the following is allowable. I can't
> believe that I didn't catch it earlier--6.3.2.3, para. 5 and 6.
>
> const char *(*getver)(void);
> getver = ((const char *)(*)(void))(intptr_t)dlsym(lib, name);
>
> It of course depends on representation.
That seems to be a very good idea to me.
(I don't know if the Austin Group participants have considered it before,
I'm a rookie subscriber.)
Thanks!
lacos
> I don't understand why the Austin Group would entertain returning to the
> casting form. If the intention is to be able to drop the 2.12.3 requirement,
> then they've just shot themselves in the foot.
By adding 2.12.3 they tried to solve a problem with one specific
function (dlsym) by creating a much more general requirement.
One which, it turns out, introduced problems of its own.
Someone else has posted a link to the austin-group-l thread on
bug 74. Let's not repeat that discussion here.
The fix for bug 74 is to remove 2.12.3 and introduce a different
fix for the dlsym problem that is specifically targeted at just
dlsym. Specifically, the new description of dlsym says:
The return value from dlsym(), cast to a pointer to the
type of the named symbol, can be used to call (in the case of
a function) or access the contents of (in the case of a data
object) the named symbol.
--
Geoff Clare <net...@gclare.org.uk>