exportfs: check fsid #878
exportfs: check fsid #878
Conversation
| @@ -359,7 +359,7 @@ cleanup_export_cache() { | |||
| break | |||
| ocf_log info "Cleanup export cache ... (try $i)" | |||
| ocf_run exportfs -f | |||
| sleep 0.5 | |||
| sleep 1 | |||
There was a problem hiding this comment.
changed due to possible bashism
|
On Thu, Nov 10, 2016 at 04:35:31AM -0800, Oyvind Albrigtsen wrote:
sleep(1) is a separate program, not implemented in shell. AFAIK, sleep 0.5 2>/dev/null || sleep 1 But are you sure that a sleep is really necessary after exportfs? |
|
On Thu, Nov 10, 2016 at 04:29:53PM +0100, Dejan Muhamedagic wrote:
I see now that sleep was already there. At any rate, I don't |
| if [ "$1" != "meta-data" ] && [ "$1" != "usage" ] && [ "$1" != "methods" ]; then | ||
| exportfs_validate_all | ||
| fi | ||
|
|
There was a problem hiding this comment.
This is superfluous (handled by ocf_rarun).
| if ! echo "$OCF_RESKEY_fsid" | grep -q -E "^[A-Za-z0-9-]*$"; then | ||
| ocf_exit_reason "$OCF_RESKEY_fsid is not a valid fsid" | ||
| return $OCF_ERR_CONFIGURED | ||
| fi |
There was a problem hiding this comment.
Hmm, I see now that there's a check for this below (ocf_is_decimal). Anything wrong with it?
There was a problem hiding this comment.
Missed that there's a check only for multiple directories. Anyway, exports(5) says here:
Other filesystems can be identified with a small integer, or a
UUID which should contain 32 hex digits and arbitrary punctua‐
tion.
That doesn't match the regexp in grep above.
There was a problem hiding this comment.
The regexp matching that description would be: '^(root|[[:xdigit:][:punct:]]*)$'. And then we'd need to hope that the description is precise. Somehow, I'd rather let this be as is, lest we introduce a regression.
oalbrigt
force-pushed
the
exportfs-check-fsid
branch
from
e206944 to
3fdbc26
Compare
|
Updated based on the comments. The sleep was changed as it was caught when I tried running checkbashisms on the script, so I figured that was worth fixing as well. $ checkbashisms heartbeat/exportfs |
|
On Thu, Nov 10, 2016 at 08:10:20AM -0800, Oyvind Albrigtsen wrote:
Well, checkbashisms is certainly wrong there. As mentioned, bash |
|
Sure. I'll reverse that part :) |
oalbrigt
force-pushed
the
exportfs-check-fsid
branch
from
3fdbc26 to
d2932d0
Compare
|
New update without the sleep-change. |
|
The regex "^[A-Fa-f0-9-]*$" still doesn't match the description from the man page. Perhaps you missed one of my earlier comments, this change may also be a regression for the existing installations. I'm not sure if the additional check is enough of a motivation for that.
|
|
Actually I think checkbashisms is somewhat right about sleep: A POSIX compliant version only accepts integers, so for compatibility, only integers should be used: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sleep.html I don't know if it's an issue in practice on any platform, though googling seems to indicate that only GNU sleep implements floating point arguments. |
oalbrigt
force-pushed
the
exportfs-check-fsid
branch
from
d2932d0 to
d1fb323
Compare
|
Oh. Seems like I missed that comment. I've now updated based on that. |
|
Yes, this may not be posix compliant, but checkbashisms should
check for bashisms, i.e. bash-specific shell constructs. That has
nothing to do with sleep(1). There is also other non-posix stuff
about which checkbashisms reports nothing.
There's a number of sleeps with floats in the repository. If we
want to "fix" that, then we'd probably need a separate function
in which we can test if sleep supports times with decimal and
if not then replace that with the closest integer number.
Still, if nobody till now complained about it...
|
|
On 10/11/16 07:57 -0800, Dejan Muhamedagic wrote:
nfs-utils in fact only have these restrictions on UUID:
http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=utils/mountd/cache.c;hb=nfs-utils-1-3-5-rc3#l260 On 16/11/16 20:06 -0800, Kristoffer Grönlund wrote:
Yes, at least since some 17 years ago: Anyway, the main problem I see here is that fsid can be formatted Jan (Poki) |
|
On 17/11/16 12:15 +0100, Jan Pokorný wrote:
s/string/strict/ :-) Jan (Poki) |
|
Please see #887 if you find it any better. |
|
Yes, #887 looks better, it's less dangerous ;-)
What I've been trying to understand is the motivation for the
change: can someone shed some light on that aspect. If there's a
problem with a fsid, it's going to be caught by exportfs.
Further, and just as important, what is the benefit of mimicking
a check which is already implemented in nfs-utils? In general, I'd
assume that such duplication is better avoided. I could spell out
the reasons, but you all certainly know them.
|
|
On 21/11/16 08:13 -0800, Dejan Muhamedagic wrote:
It's a self-defense of the agent's inner handling of the parameters, Anyway, to bring more enlightment: b. from the commit message of
PR #887 doesn't mimick any validation performed down the road, I admit it's rather lazy approach, but does this pose any issues Poki |
|
On Tue, Nov 22, 2016 at 08:15:10AM -0800, Jan Pokorný wrote:
That's of course a valid concern in principle, but note that this
If I had to read exportfs(8) and you had to prowl the nfs-utils
If exportfs cannot digest ',' then there's no harm done. But At any rate, we should probably improve that sed expression. |
|
On 23/11/16 05:06 -0800, Dejan Muhamedagic wrote:
On Tue, Nov 22, 2016 at 08:15:10AM -0800, Jan Pokorný wrote:
> On 21/11/16 08:13 -0800, Dejan Muhamedagic wrote:
>> What I've been trying to understand is the motivation for the
>> change: can someone shed some light on that aspect. If there's a
>> problem with a fsid, it's going to be caught by exportfs.
>
> It's a self-defense of the agent's inner handling of the parameters,
> as detailed in #887.
That's of course a valid concern in principle, but note that this
parameter is not really consumed by the RA (apart from the case
of multiple export), but by another program. Of course, that
doesn't mean that the RA should allow bad input to cause
malfunction, but given that the input comes from root... I mean
what would be the point of trying to break the RA by the admin
supplying some crazy value? What I'm trying to say is let's put
this in perspective of the actual use. Can we not reasonably
expect a reasonable input value?
Idea of the authorized admin being the only in charge is nice until
you recall that we live in a world far from perfect, as a recent
pacemaker vulnerability may have tought us (there was also a few pcs
web UI abusable shortcomings some years back, just to back this is
not a far cry).
It's similar to asking what would be the point of sanitizing inputs
in the door controllers when normally only those in charge can reach
them, but the harder is then a reality check:
http://www.csoonline.com/article/3050925
I hope I am not the only to see value of raising the self-defensive
bar. And also didn't get to check if there are other parts in the
project similarly affected, so hopefully someone will accept the
challenge to help in that noble goal (awk expressions seems all
OK for hearbeat agents).
[snip]
> PR #887 doesn't mimick any validation performed down the road,
If I had to read exportfs(8) and you had to prowl the nfs-utils
code to see what is being allowed then it is certainly trying to
replicate the check.
I insist it's not, commas are not disallowed by exportfs at the place
of UUID.
> but rather introduces new -- in practice negligible -- limitation
> so as to counterweight the detailed shortcomings.
^ here
We purposefully put this single artificial restriction because it's
a win at other places in the agent's inner handling. Everything
else asks for more rewriting, IMHO.
> I admit it's rather lazy approach, but does this pose any issues
> compared to sanity gains?
If exportfs cannot digest ',' then there's no harm done. But
again I do wonder if it really makes any difference.
At any rate, we should probably improve that sed expression.
+1 :-)
…--
Poki
|
|
On 24/11/16 20:59 +0100, Jan Pokorný wrote:
On 23/11/16 05:06 -0800, Dejan Muhamedagic wrote:
> If I had to read exportfs(8) and you had to prowl the nfs-utils
> code to see what is being allowed then it is certainly trying to
> replicate the check.
I insist it's not, commas are not disallowed by exportfs at the place
of UUID.
Note that strictly speaking there is no place for characters other
than hexadecimal digit characters ([0-9A-Za-z] and hyphens ('-'):
https://tools.ietf.org/html/rfc4122#section-3
so I also insist that the proposed change implying exclusion of
a comma should be negligible as hardly any admin wants to play
with open fire.
…--
Poki
|
|
If an "unauthorized admin" has gained control of the CIB, then
the game's long over; what happens with the fsid parameter is
irrelevant.
|
|
On 30/11/16 05:33 -0800, Dejan Muhamedagic wrote:
If an "unauthorized admin" has gained control of the CIB, then
the game's long over; what happens with the fsid parameter is
irrelevant.
No, it's not that simple :-)
I suggest trying it yourself under condition that you can only presume
standard minimalistic cluster deployment (common system and cluster
packages installed, but no crafted, lurking helper at the victim
machines, plus presume you cannot use any shared storage trick).
Also consider that some distros do not have do-arbitrary-things
ocf:heartbeat:anything agent, for good reasons [1], so put this
possibility aside as well.
I thing you'll find your claim not so straightforward -- actually it's
quite a good OCF design in this aspect that semi-protects against
utterly random things beyond what's defined in the agents. It's the
actual implementation (incl. the selected language) that makes some
unexpected abuses possible. And that's why I am pushing for greater
sanity regarding arbitrary command injections such as this very one,
merely as a hardening measure to limit the attack surface.
If you'll find at least two other holes, I am willing to resign in
this effort, otherwise I'll still call the fix (in whatever form)
worthwile.
[1] http://lists.clusterlabs.org/pipermail/users/2016-January/002178.html
…--
Jan (Poki)
|
No description provided.