Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exend svn and add cvs test #73

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

security-companion
Copy link
Contributor

I compared the ZAPproxy-plugin for finding hidden files with snallygaster and found some differences. Therefore I created this PR to add some missing checks.

@hannob
Copy link
Owner

hannob commented May 10, 2022

Can you explain the intention behind this?

The svn part looks like another test for svn, just done in a different way.

For CVS: I think I had previously support for this, but removed it because it practically never hit anything and only produced false positives. (Also see the "New Tests" part of https://github.com/hannob/snallygaster/blob/main/CONTRIBUTIONS.md ).

@hannob
Copy link
Owner

hannob commented May 12, 2022

Closing this for now, if you feel this has value please re-open with an explanation why you think these additional tests are valuable (ideally with some data on findings).

@hannob hannob closed this May 12, 2022
@security-companion
Copy link
Contributor Author

@hannob Sorry for not answering earlier.
I didn't know that you had problems with CVS before.

For svn: I added the second check as I found it on zaproxy. Here can be found some more details about svn. I think depending on which version of svn is used one of the 2 checks would trigger.

@hannob
Copy link
Owner

hannob commented May 13, 2022

The link you provided indicates we should be smarter in how to detect svn repos, but it's a bit scarce on details (e.g. not mentioning which versions use which format). But adding another redundant check does not seem to be the way forward.

It's been a while when I wrote this check, it seems snallygaster merely checks that .svn/entries is a number.

Do you have any examples of svn repos in old or new format that aren't detected that we could use to verify this?

For CVS: Unless you have evidence that this is in any way a relevant problem I'd just ignore it. As said, I had checks before and never found anything, I think CVS these days is just not used any more and in the days CVS was used it was unusual to use SCM systems for webpages, so I think the realistic usage of CVS for web roots is very low.

@hannob hannob reopened this May 13, 2022
@security-companion
Copy link
Contributor Author

For me it's okay to remove CVE. I'll remove it from this pull request.

For svn: I installed TortoiseSVN 1.14.3 and checked out a repo. On the disc I could find .svn/entries and .svn/wc.db
See screenshot
grafik

So if only current versions should be considered one of the 2 checks would be enough.

I'll try with older versions of svn to see how files change on disc.

@hannob
Copy link
Owner

hannob commented May 14, 2022

I'm a bit confused by the oreilly link you provided. In my tests the entries file merely contains a number, and that's what snallygaster tests. The link says it should contain all kinds of things. I didn't find any official documentation from subversion about that file's content.

It's probably worth trying with a variety of old subversion versions to see how the entries file looks and if we need to improve the test, but I currently don't have the time to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants