resource-agents
resource-agents copied to clipboard
ClusterLabs
exportfs: check fsid
On Thu, Nov 10, 2016 at 04:35:31AM -0800, Oyvind Albrigtsen wrote:
oalbrigt commented on this pull request.
@@ -359,7 +359,7 @@ cleanup_export_cache() { break ocf_log info "Cleanup export cache ... (try $i)" ocf_run exportfs -f
sleep 0.5sleep 1changed due to possible bashism
sleep(1) is a separate program, not implemented in shell. AFAIK, almost all platforms do support floats. Another possibility would be sth like:
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:
On Thu, Nov 10, 2016 at 04:35:31AM -0800, Oyvind Albrigtsen wrote:
oalbrigt commented on this pull request.
@@ -359,7 +359,7 @@ cleanup_export_cache() { break ocf_log info "Cleanup export cache ... (try $i)" ocf_run exportfs -f
sleep 0.5sleep 1changed due to possible bashism
sleep(1) is a separate program, not implemented in shell. AFAIK, almost all platforms do support floats. Another possibility would be sth like:
sleep 0.5 2>/dev/null || sleep 1
But are you sure that a sleep is really necessary after exportfs?
I see now that sleep was already there. At any rate, I don't think that this change is necessary. Or has somebody complained about it? Reported a bug?
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 possible bashism in heartbeat/exportfs line 362 (sleep only takes one integer): sleep 0.5
On Thu, Nov 10, 2016 at 08:10:20AM -0800, Oyvind Albrigtsen wrote:
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 possible bashism in heartbeat/exportfs line 362 (sleep only takes one integer): sleep 0.5
Well, checkbashisms is certainly wrong there. As mentioned, bash doesn't implement sleep and hence has nothing to do with it. Further, this change also has nothing to with the fsid check either. Still further, let's not try to fix something that isn't broken ;-)
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.
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:
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.
nfs-utils in fact only have these restrictions on UUID:
- only hex characters do count (as in isxdigit(3))
- there must be exactly 32 of them throughout the value, i.e. "punctuation" is an arbitrary int X for which isxdigit(X) == 0
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 http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/exports.c;hb=nfs-utils-1-3-5-rc3#l391
On 16/11/16 20:06 -0800, Kristoffer Grönlund wrote:
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.
Yes, at least since some 17 years ago: http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=5617251
Anyway, the main problem I see here is that fsid can be formatted so unfortunately, that it will break the validity of a sed expression later on. Being string on fsid would alleviate this issue, of new approach is to be chosen.
Jan (Poki)
On 17/11/16 12:15 +0100, Jan Pokorný wrote:
Being string on fsid would alleviate this issue, of new approach is to be chosen.
s/string/strict/ :-)
Jan (Poki)
Please see #887 if you find it any better. It's still somewhat bound to lazy side of the scale, but is a bit more systemic, IMHO.
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:
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. I'd be surprised if this was a first one of this kind. I'd be also surprised if some parameter validations in any agent were not superfluous of sorts, especially if you want to address multiple versions/implementations of the same.
Anyway, to bring more enlightment: b. from the commit message of new PR has several implications... and it's better to prevent them.
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.
PR #887 doesn't mimick any validation performed down the road, but rather introduces new -- in practice negligible -- limitation so as to counterweight the detailed shortcomings.
I admit it's rather lazy approach, but does this pose any issues compared to sanity gains?
Poki
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?
I'd be surprised if this was a first one of this kind. I'd be also surprised if some parameter validations in any agent were not superfluous of sorts, especially if you want to address multiple versions/implementations of the same.
Anyway, to bring more enlightment: b. from the commit message of new PR has several implications... and it's better to prevent them.
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.
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.
but rather introduces new -- in practice negligible -- limitation so as to counterweight the detailed shortcomings.
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.
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)