RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 1108708 - Config parser: Properly check integer values
Summary: Config parser: Properly check integer values
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: corosync
Version: 7.1
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Jan Friesse
QA Contact: Cluster QE
URL:
Whiteboard:
Depends On:
Blocks: 1167390
TreeView+ depends on / blocked
 
Reported: 2014-06-12 12:58 UTC by Jan Friesse
Modified: 2015-03-05 08:27 UTC (History)
3 users (show)

Fixed In Version: corosync-2.3.3-3.el7
Doc Type: Bug Fix
Doc Text:
Cause: User sets integer option in corosync.conf to value out of boundary (< 0 or (usually) > 2^32 - 1) Consequence: Corosync doesn't show error message. Integer is parsed and overflows. Fix: Properly check integer boundaries. Result: Corosync show error during configuration file parsing (error contains allowed integer range)
Clone Of:
: 1167390 (view as bug list)
Environment:
Last Closed: 2015-03-05 08:27:09 UTC
Target Upstream Version:
Embargoed:

Attachments (Terms of Use)
coroparse: More strict numbers parsing (9.22 KB, patch)
2014-06-12 13:00 UTC, Jan Friesse 		no flags Details | Diff
View All


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2015:0365 0 normal SHIPPED_LIVE corosync bug fix and enhancement update 2015-03-05 12:51:37 UTC

Description Jan Friesse 2014-06-12 12:58:54 UTC 
Description of problem:
Corosync doesn't properly check integers in corosync.conf for their range. It also doesn't allow enter 32-bit integer with highest bit set on 32-bit system.

Version-Release number of selected component (if applicable):
2.3.3

How reproducible:
100%

Steps to Reproduce and results:
* corosync.conf: token: -1 -> 4294967295 (no error shown)
* token: 4294967296 -> overflow -> 0 (no error shown)
* It's impossible to enter (perfectly valid) nodeid > 2^31 - 1 on 32-bit systems

Expected results:
Corosync show error on invalid integer value (check boundaries) ideally with allowed range. It's possible to use integer > 2^31 - 1 on 32-bit systems (not subject of QA. We don't support 32-bit systems any longer).
Comment 1 Jan Friesse 2014-06-12 13:00:48 UTC 
Created attachment 908110 [details]
coroparse: More strict numbers parsing

coroparse: More strict numbers parsing

Previous safe_atoi didn't check range of input values so if for example
user used -1 s token timeout, it was converted to UINT32_MAX without
letting user know.

Another safe_atoi problem was using strtol. This works pretty well on
64-bit systems, where long integer is usually 64-bits long, sadly on
32-bit systems, it is usually 32-bit long. And because strtol returns
signed integer, it was not possible to enter 32-bit value with highest
bit set.

Solution is to use strtoll which is guaranteed to be at least 64-bits
long and check value range.

Also error message now contains also information about expected value
range.

Signed-off-by: Jan Friesse <jfriesse>
Reviewed-by: Christine Caulfield <ccaulfie>
Comment 2 Jan Friesse 2014-06-12 13:04:35 UTC 
For QE: "Unit" test is csts config-strict-parse-numbers.sh
Comment 5 Jaroslav Kortus 2014-10-30 13:37:09 UTC 
With introduction of dynamic token timeout the introduced checks might not be enough. For instance setting token: 4294966646 will forbid corosync to start in 3-node cluster (4294966646+650 will result in max+1).

As the purpose of this bug was to eliminate these values from config and let the user know they are off limits, I'm flipping this back to assigned for reconsideration. Users would have to be really evil to reach these corner cases, but when we already have a bug for this... :).
Comment 6 Jan Friesse 2014-10-30 16:30:54 UTC 
Good catch. Sadly I'm really unsure if it's possible to let user know they are off limit in this weird case. Basically, during startup it's possible, but sadly during runtime it's not possible (we really do not want to crash corosync just because user added another node).

So my proposed solution is to limit maximum token timeout to uint32_max to prevent overflow. And to reach consistent behavior, not showing any extra error.

Opinions? Ideas?
Comment 7 Jaroslav Kortus 2014-10-30 17:03:00 UTC 
limiting token timeout only will not do it. We should limit also all the other variables that come into play (like the coefficient), or better said, the resulting product of them.

We could do this logic also on reload I guess, at least that worked for the join timeout vs consensus ratio, if it was not valid on reload, the reload was refused.

Is it possible to do that or are there any use cases that I don't see right now?
Comment 8 Jan Friesse 2014-10-31 16:47:51 UTC 
Oh, this is misunderstanding. By token I mean final token value. So yes, coefficient comes into play.

About join vs consensus error. Sadly it's not true (and it may be worth of BZ). Error is shown, but reload is still processed and values are used.

That's why I'm considering using highest possible values for token and related variables. Maybe with warning message for 7.1.

Opinions?
Comment 9 Jan Friesse 2014-11-24 16:04:14 UTC 
Cloning bug to 7.2, to solve token timeout overflow and add functionality to refuse reload.
Comment 13 errata-xmlrpc 2015-03-05 08:27:09 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-0365.html
Note You need to log in before you can comment on or make changes to this bug.