[dev.icinga.com #2735] global match in ./etc/apache2/icinga-web.conf.in

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/2735

Created by calestyo on 2012-06-27 14:14:42 +00:00

Assignee: (none)
Status: Resolved (closed on 2012-12-17 10:00:02 +00:00)
Target Version: 1.9
Last Update: 2012-12-17 10:06:01 +00:00 (in Redmine)

Icinga Version: 1.7.2
Icinga Web Version: 1.7.2
IDO Version: 1.7.0
OS Version: Any
DB Type: MySQL
DB Version: 5.5
Browser Version: any

---

Hi.

There is a nasty bug in ./etc/apache2/icinga-web.conf.in:
The lines:

AliasMatch web_path/modules/([A-Za-z0-9])/resources/styles/([A-Za-z0-9]\.css)$ prefix/app/modules/$1/pub/styles/$2
AliasMatch web_path/modules/([A-Za-z0-9])/resources/images/([A-Za-z_\-0-9]\.(png|gif|jpg))$ prefix/app/modules/$1/pub/images/$2

lead to any RELATIVE occasion of such strings to be matched not just absolute ones:

See https://httpd.apache.org/docs/current/mod/mod\_alias.html#aliasmatch:

Please add a "^" to the beginning of the patterns, i.e.:
AliasMatch ^web_path/modules/([A-Za-z0-9])/resources/styles/([A-Za-z0-9]\.css)$ prefix/app/modules/$1/pub/styles/$2
AliasMatch ^web_path/modules/([A-Za-z0-9])/resources/images/([A-Za-z_\-0-9]\.(png|gif|jpg))$ prefix/app/modules/$1/pub/images/$2

Marking as high, as this could remotely be even security relevant.

Cheers,
Chris.

Attachments

• safe-regexps.patch calestyo - 2012-06-29 23:19:25 +00:00
• safe-regexps.v2.patch calestyo - 2012-12-16 15:35:47 +00:00
• safe-regexps.v3.patch calestyo - 2012-12-16 15:45:11 +00:00

---

Relations:

• relates #2735
• duplicates #2735

[icinga-migration]

Updated by calestyo on 2012-06-29 23:00:26 +00:00

Actually there are more possible problems:

1. The DirectoryMatch directives:
   <DirectoryMatch @Prefix@/app/modules/\w+/pub/styles/>
   <DirectoryMatch @Prefix@/app/modules/\w+/pub/images/>

a) They also have no begin anchor "^"

b) What is good however is, the trailing "/" (after styles and images).
I would recommend that you add a comment, that this trailing / is really important.
(I write more elaborate about this in https://issues.apache.org/bugzilla/show\_bug.cgi?id=53483 in case you're interested in the backgrounds. Also a bit related is perhaps: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=679476)

c) Using \w+ (and not \w*) is also good and important.

So I'd suggest to change this to:
<DirectoryMatch ^@Prefix@/app/modules/\w+/pub/styles/>
<DirectoryMatch ^@Prefix@/app/modules/\w+/pub/images/>

2. The AliasMatch directives:
   AliasMatch web_path/modules/([A-Za-z0-9])/resources/styles/([A-Za-z0-9]\.css)$ prefix/app/modules/$1/pub/styles/$2
   AliasMatch web_path/modules/([A-Za-z0-9])/resources/images/([A-Za-z_\-0-9]\.(png|gif|jpg))$ prefix/app/modules/$1/pub/images/$2

a) As noted above, the missing beginning ^.

b) /([A-Za-z0-9])/ and /([A-Za-z0-9])/
Is really * wanted here, or not rather + ?
"*" means that e.g. also web_path/modules//resources/… could be accessed (and I guess that is not used?)!
But AFAIU it would also mean that you then really need the // in the URI, folding the / is not done automatically.

• would mean that there need to be at least on of the characters between the [].

c) /([A-Za-z0-9]\.css)$ and ([A-Za-z_\-0-9]\.(png|gif|jpg))
Using * here means that none of the characters in [] needs to be present, and therefore that the files
…/.css as well as …/.png and …/.gif and …/.jpeg themselves are matched.
This is not critical but such files are usually meant as "hidden" files in UNIX, and therefore I'd exclude them by using + instead of *.

I would further replace (png|gif|jpg) by (?:png|gif|jpg).
The ?: means that the subpattern (the thingy between (…) ) is not included in capturing, i.e. not available as $3.
This makes things a bit faster, and you don't used $3 anyway.
$1 and $2 are not affected by this.

d) The trailing $ is good and important.

So I'd suggest:
AliasMatch ^web_path/modules/([A-Za-z0-9])/resources/styles/([A-Za-z0-9]\.css)$ prefix/app/modules/$1/pub/styles/$2
AliasMatch ^web_path/modules/([A-Za-z0-9])/resources/images/([A-Za-z_\-0-9]\.(?:png|gif|jpg))$ prefix/app/modules/$1/pub/images/$2

[icinga-migration]

Updated by calestyo on 2012-06-29 23:19:25 +00:00

• File added safe-regexps.patch

Attached patch does the above changes plus

• adds "" around all patterns
• I changed the ordering of [A-Za-z_\0-9] to [A-Za-z0-9_] which makes it IMHO easier to read, and the literal needs not to be quoted anymore.

[icinga-migration]

Updated by calestyo on 2012-06-29 23:22:33 +00:00

On further side node:
In the config you use:
Options FollowSymLinks

a) symLinksIfOwnerMatch instead of FollowSymLinks should work, too, and is a tiny little bit more safe.

b) As far as I can see, there are no symlinks at all distributed by icinga-web, so I'd suggest to remove that Option alltogether.

[icinga-migration]

Updated by mfrosch on 2012-07-02 12:32:14 +00:00

b) As far as I can see, there are no symlinks at all distributed by icinga-web, so I'd suggest to remove that Option alltogether.

http://httpd.apache.org/docs/current/mod/mod\_rewrite.html

It says:
-snip-
To enable the rewrite engine in this context (( or .htaccess)), you need to set "RewriteEngine On" and "Options FollowSymLinks" must be enabled. If your administrator has disabled override of FollowSymLinks for a user's directory, then you cannot use the rewrite engine. This restriction is required for security reasons.
-snip-

[icinga-migration]

Updated by calestyo on 2012-07-02 23:26:40 +00:00

Yes you are right, I've noted that soon wards after, but forgot to upgrade, but it also work with symlinksIfOwnerMath... which is at least a very tiny bit safer in case real symlinks should move accidentally in.

And everything else in that report is obviously unaffected by this :)

[icinga-migration]

Updated by mfriedrich on 2012-10-11 22:09:48 +00:00

• Status changed from New to Feedback
• Priority changed from High to Normal
• Icinga Version set to 1
• Icinga Web Version set to 1
• IDO Version set to 1
• OS Version set to Any
• DB Type set to MySQL
• DB Version set to 5
• Browser Version set to any

[icinga-migration]

Updated by calestyo on 2012-10-11 23:55:36 +00:00

Hi Michael.

What kind of feedback to you wish? :)

Chris.

[icinga-migration]

Updated by calestyo on 2012-12-16 15:35:47 +00:00

• File added safe-regexps.v2.patch

safe-regexps.v2.patch adds the following changes to safe-regexps.patch:

• really change the ordering of [A-Za-z_\0-9] to [A-Za-z0-9_] as already said in comment #2.
• replaced FollowSymLinks by symlinksIfOwnerMath which also enables the rewriting, but is a tiny bit more secure, as talked about in comment #5

[icinga-migration]

Updated by calestyo on 2012-12-16 15:45:11 +00:00

• File added safe-regexps.v3.patch

This is taken over from bug #3499 as suggested by Jannis:

One should notice that AliasMatch does not automatically fold multiple consecutive slashes into one, as Alias or DirectoryMatch does (see also Apache bugs https://issues.apache.org/bugzilla/show\_bug.cgi?id=54307 and https://issues.apache.org/bugzilla/show\_bug.cgi?id=54308 )

So e.g. "@web_path``/modules/foo/resources/styles/bar.css" would be aliased but e.g. "``web_path``///modules/foo/resources/styles/bar.css" or e.g. a ``web_path@ in the request with // not.

For any cases of multiple / outside the @web_path@ this is usually not a problem, as people typically never directly access such files but only get there via references from icinga-web.
But when a user accidentally accessed his icinga-web e.g. via http://example.org/monitoring//icinga instead of http://example.org/monitoring/icinga it may lead to breaking URIs.

I think this is rather a cosmetic/perfectionism issue... but ideally all occurrences of / (especially also in the value of @web_path@) in the AliasMatch (not the Alias!) should be replaced by the "/+" (i.e. "one or more /") pattern.

Attached file (v3) adds the following changes to v2:

• replaces / by /+ in all regexp cases where this serves as a URI space separator.

TODO:

• Somewhere (guess in the makefile) code would need to be added, so that any occurrence of / in @web_path@ is replaced by /+ at (only!) the AliasMatch lines.

If that's all too much effort to implement, than simply stay at the v2 patch :)

Cheers,
Chris.

[icinga-migration]

Updated by mfrosch on 2012-12-17 10:00:02 +00:00

• Status changed from Feedback to Resolved

Will be fixed with #3500

Please comment there for further questions.

-Markus

[icinga-migration]

Updated by mfrosch on 2012-12-17 10:06:01 +00:00

• Target Version set to 1.9