Page MenuHomeSoftware Heritage
Differential D6823

validate input paths in the CVS loader
ClosedPublic
Actions

Authored by stsp on Dec 10 2021, 2:59 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T3691: Implement CVS loader
Commits
rDLDCVS238c9c0335af: validate input paths in the CVS loader
Summary

The CVS loader creates files on the local file system based on
paths which were read from a local copy of a CVS repository or
sent by a CVS server as part of its "cvs rlog" response.

Ensure that such paths will not be able to escape the temporary
directory which stores checked out versions of files.

Diff Detail

Repository
rDLDCVS CVS Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

stsp created this revision.Dec 10 2021, 2:59 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 10 2021, 2:59 PM
stsp added a task: T3691: Implement CVS loader.Dec 10 2021, 3:00 PM
swh-public-ci added a comment.Dec 10 2021, 3:01 PM

Build is green

Patch application report for D6823 (id=24711)

Rebasing onto a66c6b4937...

Current branch diff-target is up to date.
Changes applied before test
commit 1c48bd3b97200b069c6b66264872b8cdcdb8b868
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/82/ for more details.

Harbormaster completed remote builds in B25563: Diff 24711.Dec 10 2021, 3:01 PM
stsp requested review of this revision.Dec 10 2021, 3:01 PM
stsp mentioned this in T3691: Implement CVS loader.Dec 10 2021, 3:01 PM
stsp updated this revision to Diff 24714.Dec 10 2021, 3:05 PM
  • add missing check for "/../" to the rcsparse code path
swh-public-ci added a comment.Dec 10 2021, 3:07 PM

Build is green

Patch application report for D6823 (id=24714)

Rebasing onto a66c6b4937...

Current branch diff-target is up to date.
Changes applied before test
commit 043a20c85e956e4bf2eb4be0767d70f443d82b7c
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 15:04:55 2021 +0100

    add missing check for "/../" to the rcsparse code path

commit 1c48bd3b97200b069c6b66264872b8cdcdb8b868
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/83/ for more details.

Harbormaster completed remote builds in B25566: Diff 24714.Dec 10 2021, 3:07 PM
olasd added a subscriber: olasd.Dec 13 2021, 11:37 AM
olasd added inline comments.
swh/loader/cvs/loader.py
161–165

These statements should probably be broken out into a common method that can be unit-tested separately?

olasd added inline comments.Dec 13 2021, 11:40 AM
swh/loader/cvs/loader.py
161–165

(it's not 100% clear to me what kinds of paths are rejected that wouldn't be caught by the first ".." statement)

ardumont added a subscriber: ardumont.Dec 13 2021, 11:40 AM
ardumont added inline comments.
swh/loader/cvs/loader.py
487
ardumont added inline comments.Dec 13 2021, 6:05 PM
swh/loader/cvs/loader.py
161–165

Given the repetition and the only change between the 2 chunks (here and below) is
the log message, I second what proposed olasd.

Please, extract a function (or method) to check for the path correctness. I gather that
function will do nothing if the check passes but raises in case the check fails
otherwise.

That allows to document the function and indeed eventually cover it with dedicated
tests.

168

Also, ^ feels more neutral to me.

484–485

I'm a bit edgy when it comes to open code solely for tests.

Please, use mocks instead if you can (tentatively suggested something that probably need to be improved).
That might be less clear though.

swh/loader/cvs/tests/test_loader.py
1088–1128

using mocks instead of opening the runtime code

stsp added inline comments.Dec 16 2021, 11:37 AM
swh/loader/cvs/loader.py
161–165

(it's not 100% clear to me what kinds of paths are rejected that wouldn't be caught by the first ".." statement)

It intends to catch absolute paths that point to anywhere in the file system, instead of into the temporary directory where we store the repository. In case of rlog we obtain the list of existing paths from the server, which is why this matters. In case of rsync we mirror the repository and walk a local tree. The only possible problem there would be symlinks that point outside that tree, but that is a separate issue which I intend to fix after this change is done.

stsp added inline comments.Dec 17 2021, 11:38 AM
swh/loader/cvs/tests/test_loader.py
1088–1128

I need a way to ensure that the mocker returns the file only the first time it gets called. Afterwards it must return None. To see why, search loader.py for the string "Try to fetch more rlog data from this Attic directory."

stsp updated this revision to Diff 24862.Dec 17 2021, 11:41 AM

move path-safety check into a helper function

This does not yet address the test code problem pointed out by ardumont.

swh-public-ci added a comment.Dec 17 2021, 11:43 AM

Build is green

Patch application report for D6823 (id=24862)

Rebasing onto cbde981229...

First, rewinding head to replay your work on top of it...
Applying: validate input paths in the CVS loader
Changes applied before test
commit 7e22b880957575b02068d345e1bf183b7c80cc98
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/89/ for more details.

Harbormaster completed remote builds in B25729: Diff 24862.Dec 17 2021, 11:43 AM
stsp added inline comments.Dec 17 2021, 12:07 PM
swh/loader/cvs/tests/test_loader.py
1088–1128

Looks like I can use mock.side_effect to achieve this.
I have cobbled something together that seems to work.

stsp updated this revision to Diff 24863.Dec 17 2021, 12:08 PM

new version which uses a mock class for the test

swh-public-ci added a comment.Dec 17 2021, 12:10 PM

Build is green

Patch application report for D6823 (id=24863)

Rebasing onto cbde981229...

First, rewinding head to replay your work on top of it...
Applying: validate input paths in the CVS loader
Changes applied before test
commit e4244eef1454734f714faf839eef407f027fbe64
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/90/ for more details.

Harbormaster completed remote builds in B25730: Diff 24863.Dec 17 2021, 12:10 PM
stsp updated this revision to Diff 24864.Dec 17 2021, 12:12 PM

remove self.log.info() call that was used for debugging

swh-public-ci added a comment.Dec 17 2021, 12:13 PM

Build is green

Patch application report for D6823 (id=24864)

Rebasing onto cbde981229...

First, rewinding head to replay your work on top of it...
Applying: validate input paths in the CVS loader
Changes applied before test
commit 41e00281bdeb8d24a8af21362388d7fabba5db8a
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/91/ for more details.

Harbormaster completed remote builds in B25731: Diff 24864.Dec 17 2021, 12:13 PM
ardumont added a comment.Dec 17 2021, 3:36 PM

@stsp Thanks a lot!

I've suggested one more consistent change to add the missing coverage part and then i'll let you be ;)

swh/loader/cvs/loader.py
151

Can you add the other unsafe rlog file which tests this (I've called it "unsafe_rlog_wrong_arborescence.rlog" below)?

I've proposed a patch below to deal with the testing part slight adaptation (to avoid duplication).

swh/loader/cvs/tests/test_loader.py
1088–1123

A couple of suggestions to deal with multiple unsafe rlog files (the ones currently covered by the code).

ardumont mentioned this in T3788: staging: Deploy cvs loader v0.1.Dec 17 2021, 3:49 PM
ardumont added a comment.Jan 4 2022, 4:51 PM

@stsp did you see my previous comment by any chance?

ardumont accepted this revision.Jan 6 2022, 12:17 PM

We'll figure out my last suggestion later.

This revision is now accepted and ready to land.Jan 6 2022, 12:17 PM
stsp updated this revision to Diff 24962.Jan 6 2022, 12:26 PM

new patch with some suggestions from ardumont applied

swh-public-ci added a comment.Jan 6 2022, 12:28 PM

Build is green

Patch application report for D6823 (id=24962)

Rebasing onto cbde981229...

First, rewinding head to replay your work on top of it...
Applying: validate input paths in the CVS loader
Changes applied before test
commit 113dcd68d355dd65af3a090d3230010cce542dde
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/92/ for more details.

Harbormaster completed remote builds in B25833: Diff 24962.Jan 6 2022, 12:28 PM
stsp updated this revision to Diff 24964.Jan 6 2022, 12:36 PM

rebased patch

swh-public-ci added a comment.Jan 6 2022, 12:37 PM

Build is green

Patch application report for D6823 (id=24964)

Rebasing onto cbde981229...

Current branch diff-target is up to date.
Changes applied before test
commit 238c9c0335af32fa6e07a1bad7017c116cd99628
Author: Stefan Sperling <stsp@stsp.name>
Date:   Fri Dec 10 13:36:52 2021 +0100

    validate input paths in the CVS loader
    
    The CVS loader creates files on the local file system based on
    paths which were read from a local copy of a CVS repository or
    sent by a CVS server as part of its "cvs rlog" response.
    
    Ensure that such paths will not be able to escape the temporary
    directory which stores checked out versions of files.

See https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/93/ for more details.

Harbormaster completed remote builds in B25835: Diff 24964.Jan 6 2022, 12:37 PM
Closed by commit rDLDCVS238c9c0335af: validate input paths in the CVS loader (authored by stsp). · Explain WhyJan 6 2022, 12:38 PM
This revision was automatically updated to reflect the committed changes.
stsp added a commit: rDLDCVS238c9c0335af: validate input paths in the CVS loader.

Revision Contents
Changeset List

PathSize
swh/
loader/
cvs/
28 lines
tests/
data/
103 lines
18 lines
65 lines

Diff 24965

View Options

swh/loader/cvs/loader.py

Loading...
View Options

swh/loader/cvs/tests/data/unsafe_rlog_with_unsafe_relative_path.rlog

Loading...
View Options

swh/loader/cvs/tests/data/unsafe_rlog_wrong_arborescence.rlog

Loading...
View Options

swh/loader/cvs/tests/test_loader.py

Loading...
About · Developer information · Legal