Skip to content

Conversation

@JonBendtsen
Copy link
Contributor

securing apache/dolibarr installation by letting apache deny access to lots of file types and some directories we can probably safely deny access to

Related to TALK #53

@cibero42
Copy link
Contributor

Hey @JonBendtsen ,

Isn't this PR overlapping with #54?

@cibero42
Copy link
Contributor

Why are you readding PR #54, and then removing it?

Won't the end result be the lack of PR #54?

@JonBendtsen
Copy link
Contributor Author

Hey @JonBendtsen ,

Isn't this PR overlapping with #54?

I would say a sort of soft no. Yeah the overall goal is the same, securing and hardening the Apache configuration, but it uses different methods, and not only are the sub-goal is different, they can be implemented independently.

(I am a proponent for small limited independent PR)

@cibero42
Copy link
Contributor

Hey @JonBendtsen ,

Isn't this PR overlapping with #54?

I would say a sort of soft no. Yeah the overall goal is the same, securing and hardening the Apache configuration, but it uses different methods, and not only are the sub-goal is different, they can be implemented independently.

(I am a proponent for small limited independent PR)

I agree with you on that - it's just that the first commit for this PR also adds the same code of #54. And then, on the second commit, you remove that.

Won't this mess up with PR #54?

@JonBendtsen
Copy link
Contributor Author

JonBendtsen commented Mar 24, 2025

the first commit for this PR also adds the same code of #54.

yeah, that was a mistake, I thought I had started from main branch, but I had not.

And then, on the second commit, you remove that.

Won't this mess up with PR #54?

I don't think it will mess up PR #54, but I do think that I will have to rebase, possibly even manually rebase either #54 or #55 - which ever is merged first.

but is is just because both of them change the same line in the same file

Copy link
Contributor

@cibero42 cibero42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to comments I did

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Denying MS Office, Libre Office, or even CSV might be a bad idea, as most companies rely on those types of documents

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cibero42

yes, but I do no think these files are served directly through Apache as static files, I think they are served as files through PHP - because else you can not have Dolibarr access control to these files.

How do we figure out? Well, we test it, and we test it all the places that we download files in Dolibarr.

Examples

EXPORT  /document.php?modulepart=export&file=export_adherent_1.csv&entity=1
DOCUMENTS    /document.php?modulepart=ecm&attachment=1&file=Manuals%2Finvitational_brutto_liste.ods
ORDER PDF      /document.php?hashp=********************

Claim all files in Dolibarr are downloaded through document.php not through apache, and thus we can easily without issues ban any other file than .php and .html files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jon,

Thanks for the explanation! If that's the case, it's better to include other MS Office and LibreOffice formats, no?

If you prefer, I can try to find a comprehensive list for you and post here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Denying MS Office, Libre Office, or even CSV might be a bad idea, as most companies rely on those types of documents

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify why this line is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cibero42 yes. When we build the image using the Dockerfile, then this line copies line you ask about into the image using this line.
COPY apache-deny.conf /etc/apache2/conf-enabled/deny.conf

the line you ask about keeps many different kinds of apache deny files in a central folder, and then it combines them into 1 single file which is easier to COPY into the image rather than many different COPY lines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cibero42 I think that there is a potential downside to this apache deny method... I think it costs performance because I believe that Apache has to basically do this check on each and every connection :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, maybe I have a better idea...

Block all by default, and allow only authorized extensions - maybe this way, we won't need to think about the various extensions to block, but only the ones that should be accepted. Perhaps that won't degrade performance a lot since Apache will check a smaller list. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I love the default Block, and allow the right ones.

But Which ones do we need to allow? Because we need to nail 100% correct to avoid issue.

How do we test that? And how do we automate that test?

JonB

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the issue.

I think that we can verify the requests done by the browser debugger and maybe check some things on Doxygen.

Perhaps one of Dolibarr's maintainers could help on that too

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.

2 participants