Bug 10984 - spoolss IDL response marshalling fails when returning error without clearing info
spoolss IDL response marshalling fails when returning error without clearing ...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Printing
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 11080 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-04 17:55 UTC by David Disseldorp
Modified: 2015-02-03 20:10 UTC (History)
7 users (show)

See Also:


Attachments
patchset for master (13.51 KB, patch)
2014-12-18 19:14 UTC, David Disseldorp
ddiss: review? (asn)
Details
v4-1-test patchset, cherry-picked from master (12.64 KB, patch)
2015-01-08 10:46 UTC, David Disseldorp
ddiss: review? (asn)
asn: review+
Details
v4-2-test patchset, cherry-picked from master (12.64 KB, patch)
2015-01-08 10:59 UTC, David Disseldorp
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2014-12-04 17:55:58 UTC
I've flagged this as a printing bug, but may be better fixed in (P)IDL.

The spoolss GetJob call is defined via the following IDL:

WERROR spoolss_GetJob(
        [in,ref] policy_handle *handle,
        [in]     uint32 job_id,
        [in]     uint32 level,
        [in,unique] DATA_BLOB *buffer,
        [in]     uint32 offered,
        [out,unique,subcontext(4),subcontext_size(offered),switch_is(level)] spoolss_JobInfo *info,
        [out,ref] uint32 *needed
);

In handling a spoolss GetJob request, the _spoolss_GetJob() handler may return an immediate error if one of the input parameters is invalid. If this is done without zeroing the pre-allocated @info pointer, then api_spoolss_GetJob() will attempt to marshall @info, which in the case of an @offered value of zero results in a marshalling error:

  ndr_push_error(7): Bad subcontext (PUSH) content_size 64 is larger than size_is(0)

The logic leading up to this error is as follows:

static bool api_spoolss_GetJob(struct pipes_struct *p)
{
...
        ZERO_STRUCT(r->out);
        r->out.info = talloc_zero(r, union spoolss_JobInfo);
...
        r->out.needed = talloc_zero(r, uint32_t);
...
        r->out.result = _spoolss_GetJob(p, r);
...
        ndr_err = call->ndr_push(push, NDR_OUT, r);
...

static enum ndr_err_code ndr_push_spoolss_GetJob(struct ndr_push *ndr, int flags, const struct spoolss_GetJob *r)
{
...
        if (flags & NDR_OUT) {
                NDR_CHECK(ndr_push_unique_ptr(ndr, r->out.info));
                if (r->out.info) {
                        {
                                struct ndr_push *_ndr_info;
                                NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_info, 4, r->in.offered));
                                NDR_CHECK(ndr_push_set_switch_value(_ndr_info, r->out.info, r->in.level));
                                NDR_CHECK(ndr_push_spoolss_JobInfo(_ndr_info, NDR_SCALARS|NDR_BUFFERS, r->out.info));
                                NDR_CHECK(ndr_push_subcontext_end(ndr, _ndr_info, 4, r->in.offered));
                        }
...

The obvious fix is to ensure that @r->out.info is zeroed in all _spoolss_GetJob() error cases, but it may make more sense to have the IDL layer catch occurrences where an output buffer can't be marshalled due to a size restriction carried in the input parameters.

A selftest (TESTS="samba3.rpc.spoolss") reproducer for this bug can be found at:

https://git.samba.org/?p=ddiss/samba.git;a=commitdiff;h=da5c2c05c760b31997b5bb78a3ad2cc9571cfad9
Comment 1 David Disseldorp 2014-12-04 18:40:34 UTC
I expect the following functions also need to be checked:

_spoolss_GetPrinterDriver2()
_spoolss_GetForm()
_spoolss_GetPrintProcessorDirectory()
_spoolss_GetPrinterDriverDirectory()
_spoolss_GetPrinterDriver()
_spoolss_GetPrinter()
Comment 2 David Disseldorp 2014-12-18 19:14:22 UTC
Created attachment 10551 [details]
patchset for master
Comment 3 David Disseldorp 2014-12-18 19:16:51 UTC
(In reply to David Disseldorp from comment #1)

_spoolss_GetPrinterDriver() doesn't need to be changed, as it sets p->fault_state, which ensures that no attempt is made to marshal the response.
Comment 4 Guenther Deschner 2014-12-18 19:29:24 UTC
David, can you please explain why all that manual setting to NULL of r->out.info in the error paths is necessary after the preceding 
"r->out.info = SPOOLSS_BUFFER_OK(r->out.info, NULL);" which is right at the end of these functions ?
Comment 5 David Disseldorp 2014-12-19 11:36:18 UTC
(In reply to Guenther Deschner from comment #4)
David, can you please explain why all that manual setting to NULL of r->out.info in the error paths is necessary after the preceding 
"r->out.info = SPOOLSS_BUFFER_OK(r->out.info, NULL);" which is right at the end of these functions ?

The SPOOLSS_BUFFER_OK() calls are in separate return paths, and are used for bounds checking of the response buffer.
Comment 6 David Disseldorp 2015-01-08 10:46:43 UTC
Created attachment 10592 [details]
v4-1-test patchset, cherry-picked from master
Comment 7 David Disseldorp 2015-01-08 10:59:50 UTC
Created attachment 10593 [details]
v4-2-test patchset, cherry-picked from master
Comment 8 Andreas Schneider 2015-01-08 15:18:21 UTC
Karolin, please add the patches to the relevant branches. Thanks!
Comment 9 Karolin Seeger 2015-01-14 20:59:14 UTC
(In reply to Andreas Schneider from comment #8)
Pushed to autobuild-v4-[1|2]-test.
Comment 10 Karolin Seeger 2015-01-16 20:05:44 UTC
Pushed to v4-1-test and v4-2-test.
Closing out bug report.

Thanks!
Comment 11 Marc Muehlfeld 2015-01-28 22:46:24 UTC
*** Bug 11080 has been marked as a duplicate of this bug. ***