[PATCH] Change system handling in smbd

[Andrew Bartlett]

Andreas,

I can't find a record of asking you about this patch, but before the
beta, I was working on this tree:

https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-system

In short, it make the system token just that, SID_NT_SYSTEM.  The unix
portion remains as is, the sec_initial_uid() and it's primary group.

I wondered if you could confirm that this doesn't break the special case
that started you on modifying this area?  

Thanks,

Andrew Bartlett
--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andreas Schneider-15]

On Monday 11 June 2012 19:53:18 Andrew Bartlett wrote:
> Andreas,

Andrew,

> I can't find a record of asking you about this patch, but before the
> beta, I was working on this tree:
>
> https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix
> -system
>
> In short, it make the system token just that, SID_NT_SYSTEM.  The unix
> portion remains as is, the sec_initial_uid() and it's primary group.
>
> I wondered if you could confirm that this doesn't break the special case
> that started you on modifying this area?

the plan was to completely fake this structure. So don't call and getpw*
functions. Only use sec_initial_uid() and sec_initial_gid().

utok and security_token should be enough.

Thanks,


        -- andreas

--
Andreas Schneider                   GPG-ID: F33E3FC6
Samba Team                             [hidden email]
www.samba.org

[Andrew Bartlett]

On Mon, 2012-06-11 at 12:13 +0200, Andreas Schneider wrote:

> On Monday 11 June 2012 19:53:18 Andrew Bartlett wrote:
> > Andreas,
>
> Andrew,
>
> > I can't find a record of asking you about this patch, but before the
> > beta, I was working on this tree:
> >
> > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix
> > -system
> >
> > In short, it make the system token just that, SID_NT_SYSTEM.  The unix
> > portion remains as is, the sec_initial_uid() and it's primary group.
> >
> > I wondered if you could confirm that this doesn't break the special case
> > that started you on modifying this area?
>
> the plan was to completely fake this structure. So don't call and getpw*
> functions. Only use sec_initial_uid() and sec_initial_gid().
>
> utok and security_token should be enough.

Thanks, I'll proceed with fully faking it up.  That can then avoid all
the DB calls.

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andrew Bartlett]

On Mon, 2012-06-11 at 12:13 +0200, Andreas Schneider wrote:

> On Monday 11 June 2012 19:53:18 Andrew Bartlett wrote:
> > Andreas,
>
> Andrew,
>
> > I can't find a record of asking you about this patch, but before the
> > beta, I was working on this tree:
> >
> > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix
> > -system
> >
> > In short, it make the system token just that, SID_NT_SYSTEM.  The unix
> > portion remains as is, the sec_initial_uid() and it's primary group.
> >
> > I wondered if you could confirm that this doesn't break the special case
> > that started you on modifying this area?
>
> the plan was to completely fake this structure. So don't call and getpw*
> functions. Only use sec_initial_uid() and sec_initial_gid().
>
> utok and security_token should be enough.

How about the attached?

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andrew Bartlett]

On Tue, 2012-06-12 at 17:16 +1000, Andrew Bartlett wrote:

> On Mon, 2012-06-11 at 12:13 +0200, Andreas Schneider wrote:
> > On Monday 11 June 2012 19:53:18 Andrew Bartlett wrote:
> > > Andreas,
> >
> > Andrew,
> >
> > > I can't find a record of asking you about this patch, but before the
> > > beta, I was working on this tree:
> > >
> > > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix
> > > -system
> > >
> > > In short, it make the system token just that, SID_NT_SYSTEM.  The unix
> > > portion remains as is, the sec_initial_uid() and it's primary group.
> > >
> > > I wondered if you could confirm that this doesn't break the special case
> > > that started you on modifying this area?
> >
> > the plan was to completely fake this structure. So don't call and getpw*
> > functions. Only use sec_initial_uid() and sec_initial_gid().
> >
> > utok and security_token should be enough.
>
> How about the attached?

Updated patches (currently under test) are at
https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-system

Andrew Bartlett
--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andreas Schneider-2]

On Tue 12.Jun.12 18:02, Andrew Bartlett wrote:
>Updated patches (currently under test) are at
>https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-system
>

They look fine for me. Feel free to push them.


Thanks,


     -- andreas

[Andrew Bartlett]

On Tue, 2012-06-12 at 10:17 +0200, Andreas Schneider wrote:
>
> On Tue 12.Jun.12 18:02, Andrew Bartlett wrote:
> >Updated patches (currently under test) are at
> >https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-system
> >
>
> They look fine for me. Feel free to push them.

There is a problem.  While I can fix up the build issue, removing the
getpwuid() and passdb dependency is harder, as it breaks spoolss tests.

Annoyingly, this doesn't seem happen in an individual test, so far I've
only reproduced it during a full make test:

[485/671 in 13m52s] samba3.rpc.spoolss.driver(s3dc)
Using seed 1339552309
Testing OpenPrinter(\\127.0.0.2)
Testing GetPrinterData(Architecture)
Testing ClosePrinter
Testing OpenPrinter(\\127.0.0.2)
Testing GetPrinterData(Architecture)
Testing ClosePrinter
Connecting printer driver share 'print$' on '127.0.0.2'
Uploading printer driver files to \\127.0.0.2\print$
Uploading /usr/share/cups/drivers/x64/pscript5.dll to x64\pscript5.dll
Uploading /usr/share/cups/drivers/x64/cups6.ppd to x64\cups6.ppd
Uploading /usr/share/cups/drivers/x64/cupsui6.dll to x64\cupsui6.dll
Uploading /usr/share/cups/drivers/x64/pscript.hlp to x64\pscript.hlp
Testing AddPrinterDriverEx(torture_driver_deleter) level: 3,
environment: 'Windows x64'
Testing EnumPrinterDrivers(Windows x64) level 3
Connecting printer driver share 'print$' on '127.0.0.2'
Uploading printer driver files to \\127.0.0.2\print$
Uploading /usr/share/cups/drivers/x64/pscript5.dll to x64\pscript5.dll
Uploading /usr/share/cups/drivers/x64/cupsps6.dll to x64\cupsps6.dll
Uploading /usr/share/cups/drivers/x64/cups6.ini to x64\cups6.ini
Uploading /usr/share/cups/drivers/x64/pscript.hlp to x64\pscript.hlp
Testing AddPrinterDriverEx(torture_driver_deleterin) level: 3,
environment: 'Windows x64'
Testing EnumPrinterDrivers(Windows x64) level 3
Testing DeletePrinterDriverEx(torture_driver_deleter)
Testing DeletePrinterDriverEx(torture_driver_deleter)
Connecting printer driver share 'print$' on '127.0.0.2'
checking non-existent driver files at \\127.0.0.2\print$
checking for driver file at x64\3\cups6.ppd
WARNING!: ../source4/torture/rpc/spoolss.c:8981: Expression
`check_printer_driver_file(tctx, cli, d, d->info8.data_file) ==
expect_exist' failed: failed data_file check
UNEXPECTED(failure):
samba3.rpc.spoolss.driver.driver.del_driver_unused_files(s3dc)
REASON: _StringException:
_StringException: ../source4/torture/rpc/spoolss.c:9607: Expression
`check_printer_driver_files(tctx, dcerpc_server_name(p), d1, 0)' failed:
printer driver file check failed

FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

A summary with detailed information can be found in:
  ./st/summary
SELFTEST FAILED

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[David Disseldorp-2]

Hi Andrew,

On Fri, 15 Jun 2012 22:10:50 +1000
Andrew Bartlett <[hidden email]> wrote:

> There is a problem.  While I can fix up the build issue, removing the
> getpwuid() and passdb dependency is harder, as it breaks spoolss tests.
>
> Annoyingly, this doesn't seem happen in an individual test, so far I've
> only reproduced it during a full make test:
>
> [485/671 in 13m52s] samba3.rpc.spoolss.driver(s3dc)

In handling the DeletePrinterDriverEx request spoolss needs to issue IO
to the print$ share, to do so it creates a fake connection struct.

This change-set fails in share_access_check() when checking the print$
share DACL (grant full access to global_sid_World) against the system
token, as the system token does not include global_sid_World.

Adding global_sid_World to the system security token allows the test to
pass, however this should not be necessary.
DeletePrinterDriverEx should be handled by the RPC server as the user
connected to the spoolss pipe (unless force user/group is set), the
system security token should be irrelevant through this code path IIUC.

Cheers, David

[Andreas Schneider-2]

On Mon 18.Jun.12 16:29, David Disseldorp wrote:

>Hi Andrew,
>
>On Fri, 15 Jun 2012 22:10:50 +1000
>Andrew Bartlett <[hidden email]> wrote:
>
>> There is a problem.  While I can fix up the build issue, removing the
>> getpwuid() and passdb dependency is harder, as it breaks spoolss tests.
>>
>> Annoyingly, this doesn't seem happen in an individual test, so far I've
>> only reproduced it during a full make test:
>>
>> [485/671 in 13m52s] samba3.rpc.spoolss.driver(s3dc)
>
>In handling the DeletePrinterDriverEx request spoolss needs to issue IO
>to the print$ share, to do so it creates a fake connection struct.
>
>This change-set fails in share_access_check() when checking the print$
>share DACL (grant full access to global_sid_World) against the system
>token, as the system token does not include global_sid_World.
>
>Adding global_sid_World to the system security token allows the test to
>pass, however this should not be necessary.
>DeletePrinterDriverEx should be handled by the RPC server as the user
>connected to the spoolss pipe (unless force user/group is set), the
>system security token should be irrelevant through this code path IIUC.

Hey,

so delete_drivers() needs to be called with the session_info from the
connecting user, else it doesn't make sense. The following branch works
just fine for me.

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/fix-system

Cheers,


     -- andreas

[David Disseldorp-2]

On Tue, 19 Jun 2012 09:53:30 +0200
Andreas Schneider <[hidden email]> wrote:

> so delete_drivers() needs to be called with the session_info from the
> connecting user, else it doesn't make sense. The following branch works
> just fine for me.
>
> https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/fix-system

Your fix looks good Andreas.

Cheers, David