Hello:
I have a Windows 2003 Server that is causing rpcclient to SEGV via the
following command:
$ rpcclient -U Administrator%foobar -c 'netshareenum 502' <server>
...
type: 0x6269: SEC_DESC_OWNER_DEFAULTED SEC_DESC_DACL_DEFAULTED
SEC_DESC_SACL_DEFAULTED SEC_DESC_DACL_TRUSTED
SEC_DESC_SACL_AUTO_INHERIT_REQ SEC_DESC_SACL_PROTECTED
SEC_DESC_RM_CONTROL_VALID
SACL
Segmentation fault (core dumped)
I did a little poking and it seems that the issue is here:
source3/rpcclient/cmd_srvsvc.c:
384 case
502:
385 for (i = 0; i <
totalentries;i++)
386
display_share_info_502(&info_ctr.ctr.ctr502->array[i]);
387 break;
Sorry for the formatting. But the NDR code yanks out 35 SHARE_INFO_502* *
entries* *but the array size NDR code calculates only 34. Since
"totalentries" is one entry too big, it causes rpcclient to go past
the end
of the ctr502 array and SEGV.
See here:
(gdb) p *info_ctr.ctr.ctr502
$9 = {
count = 34,
array = 0x67a140
}
(gdb) p totalentries
$10 = 35
Commit history shows that when the specific enum shares got unionized this
loop changed to use "totalentries" intsead of
"ctr.num_entries," which
without looking into it might have been equivalent to "count."
It would seem to me that "totalentries" really has to be bounds
checked
here else you can fall into this trap.
I know this is ugly, but couldn't something be done like
offsetof(ctr.share.infoXX, count) to verify that that the array size and
total entries match. Or perhaps even better check this bounds condition
during the NDR pull out unmarshalling code? (that is what I would vote for
since it puts less of a burden on the callee but there may be cases where
knowing the total entries vs what is in the array is useful, not sure...).
I am by no means a Samba expert but any insight into this issue would be
greatly appreciated.
Cheers!
-aps
On Mon, Aug 19, 2013 at 6:21 PM, pisymbol . <pisymbol at gmail.com> wrote:> Hello: > > I have a Windows 2003 Server that is causing rpcclient to SEGV via the > following command: > > $ rpcclient -U Administrator%foobar -c 'netshareenum 502' <server> > ... > type: 0x6269: SEC_DESC_OWNER_DEFAULTED SEC_DESC_DACL_DEFAULTED > SEC_DESC_SACL_DEFAULTED SEC_DESC_DACL_TRUSTED > SEC_DESC_SACL_AUTO_INHERIT_REQ SEC_DESC_SACL_PROTECTED > SEC_DESC_RM_CONTROL_VALID > SACL > Segmentation fault (core dumped) > > I did a little poking and it seems that the issue is here: > > source3/rpcclient/cmd_srvsvc.c: > 384 case > 502: > > 385 for (i = 0; i < > totalentries;i++) > > 386 > display_share_info_502(&info_ctr.ctr.ctr502->array[i]); > 387 break; > > Sorry for the formatting. But the NDR code yanks out 35 SHARE_INFO_502* * > entries* *but the array size NDR code calculates only 34. Since > "totalentries" is one entry too big, it causes rpcclient to go past the end > of the ctr502 array and SEGV. > > See here: > > (gdb) p *info_ctr.ctr.ctr502 > $9 = { > count = 34, > array = 0x67a140 > } > (gdb) p totalentries > $10 = 35 > > Commit history shows that when the specific enum shares got unionized this > loop changed to use "totalentries" intsead of "ctr.num_entries," which > without looking into it might have been equivalent to "count." > > It would seem to me that "totalentries" really has to be bounds checked > here else you can fall into this trap. > > I know this is ugly, but couldn't something be done like > offsetof(ctr.share.infoXX, count) to verify that that the array size and > total entries match. Or perhaps even better check this bounds condition > during the NDR pull out unmarshalling code? (that is what I would vote for > since it puts less of a burden on the callee but there may be cases where > knowing the total entries vs what is in the array is useful, not sure...). > > I am by no means a Samba expert but any insight into this issue would be > greatly appreciated. >Uh crap my bad, this is Fedora 13 x86-64, 3.6 stable. -aps
On Mon, Aug 19, 2013 at 06:21:02PM -0400, pisymbol . wrote:> Hello: > > I have a Windows 2003 Server that is causing rpcclient to SEGV via the > following command: > > $ rpcclient -U Administrator%foobar -c 'netshareenum 502' <server> > ... > type: 0x6269: SEC_DESC_OWNER_DEFAULTED SEC_DESC_DACL_DEFAULTED > SEC_DESC_SACL_DEFAULTED SEC_DESC_DACL_TRUSTED > SEC_DESC_SACL_AUTO_INHERIT_REQ SEC_DESC_SACL_PROTECTED > SEC_DESC_RM_CONTROL_VALID > SACL > Segmentation fault (core dumped) > > I did a little poking and it seems that the issue is here: > > source3/rpcclient/cmd_srvsvc.c: > 384 case > 502: > > 385 for (i = 0; i < > totalentries;i++) > > 386 > display_share_info_502(&info_ctr.ctr.ctr502->array[i]); > 387 break; > > Sorry for the formatting. But the NDR code yanks out 35 SHARE_INFO_502* * > entries* *but the array size NDR code calculates only 34. Since > "totalentries" is one entry too big, it causes rpcclient to go past the end > of the ctr502 array and SEGV. > > See here: > > (gdb) p *info_ctr.ctr.ctr502 > $9 = { > count = 34, > array = 0x67a140 > } > (gdb) p totalentries > $10 = 35 > > Commit history shows that when the specific enum shares got unionized this > loop changed to use "totalentries" intsead of "ctr.num_entries," which > without looking into it might have been equivalent to "count." > > It would seem to me that "totalentries" really has to be bounds checked > here else you can fall into this trap. > > I know this is ugly, but couldn't something be done like > offsetof(ctr.share.infoXX, count) to verify that that the array size and > total entries match. Or perhaps even better check this bounds condition > during the NDR pull out unmarshalling code? (that is what I would vote for > since it puts less of a burden on the callee but there may be cases where > knowing the total entries vs what is in the array is useful, not sure...). > > I am by no means a Samba expert but any insight into this issue would be > greatly appreciated.Can you log a bug and attach the specific packet trace that shows this problem. I'd really like to look at this in more detail. Also, exactly what version of Samba are you running ? Jeremy.
On Mon, Aug 19, 2013 at 06:21:02PM -0400, pisymbol . wrote:> Hello: > > I have a Windows 2003 Server that is causing rpcclient to SEGV via the > following command: > > $ rpcclient -U Administrator%foobar -c 'netshareenum 502' <server> > ... > type: 0x6269: SEC_DESC_OWNER_DEFAULTED SEC_DESC_DACL_DEFAULTED > SEC_DESC_SACL_DEFAULTED SEC_DESC_DACL_TRUSTED > SEC_DESC_SACL_AUTO_INHERIT_REQ SEC_DESC_SACL_PROTECTED > SEC_DESC_RM_CONTROL_VALID > SACL > Segmentation fault (core dumped) > > I did a little poking and it seems that the issue is here: > > source3/rpcclient/cmd_srvsvc.c: > 384 case > 502: > > 385 for (i = 0; i < > totalentries;i++) > > 386 > display_share_info_502(&info_ctr.ctr.ctr502->array[i]); > 387 break; > > Sorry for the formatting. But the NDR code yanks out 35 SHARE_INFO_502* * > entries* *but the array size NDR code calculates only 34. Since > "totalentries" is one entry too big, it causes rpcclient to go past the end > of the ctr502 array and SEGV. > > See here: > > (gdb) p *info_ctr.ctr.ctr502 > $9 = { > count = 34, > array = 0x67a140 > } > (gdb) p totalentries > $10 = 35 > > Commit history shows that when the specific enum shares got unionized this > loop changed to use "totalentries" intsead of "ctr.num_entries," which > without looking into it might have been equivalent to "count." > > It would seem to me that "totalentries" really has to be bounds checked > here else you can fall into this trap. > > I know this is ugly, but couldn't something be done like > offsetof(ctr.share.infoXX, count) to verify that that the array size and > total entries match. Or perhaps even better check this bounds condition > during the NDR pull out unmarshalling code? (that is what I would vote for > since it puts less of a burden on the callee but there may be cases where > knowing the total entries vs what is in the array is useful, not sure...). > > I am by no means a Samba expert but any insight into this issue would be > greatly appreciated.Actually I think that "totalentries" is just the wrong thing to use here. Can you try the following patch to see if it fixes the problem ? Jeremy. -------------- next part -------------- diff --git a/source3/rpcclient/cmd_srvsvc.c b/source3/rpcclient/cmd_srvsvc.c index 0d67639..e5fa065 100644 --- a/source3/rpcclient/cmd_srvsvc.c +++ b/source3/rpcclient/cmd_srvsvc.c @@ -273,6 +273,7 @@ static WERROR cmd_srvsvc_net_share_enum_int(struct rpc_pipe_client *cli, WERROR result; NTSTATUS status; uint32_t totalentries = 0; + uint32_t count = 0; uint32_t resume_handle = 0; uint32_t *resume_handle_p = NULL; uint32 preferred_len = 0xffffffff, i; @@ -374,15 +375,18 @@ static WERROR cmd_srvsvc_net_share_enum_int(struct rpc_pipe_client *cli, switch (info_level) { case 1: - for (i = 0; i < totalentries; i++) + count = info_ctr.ctr.ctr1->count; + for (i = 0; i < count; i++) display_share_info_1(&info_ctr.ctr.ctr1->array[i]); break; case 2: - for (i = 0; i < totalentries; i++) + count = info_ctr.ctr.ctr2->count; + for (i = 0; i < count; i++) display_share_info_2(&info_ctr.ctr.ctr2->array[i]); break; case 502: - for (i = 0; i < totalentries; i++) + count = info_ctr.ctr.ctr502->count; + for (i = 0; i < count; i++) display_share_info_502(&info_ctr.ctr.ctr502->array[i]); break; default: