After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691500 - [PATCH] egg-armor: Fix memrchr() call with negative string length
[PATCH] egg-armor: Fix memrchr() call with negative string length
Status: RESOLVED FIXED
Product: gnome-keyring
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME keyring maintainer(s)
GNOME keyring maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-01-10 21:40 UTC by Colin Walters
Modified: 2013-01-13 08:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] egg-armor: Fix memrchr() call with negative string length (1.20 KB, patch)
2013-01-10 21:40 UTC, Colin Walters
none Details | Review
Patch with tests and fixes bad practice for git master (8.01 KB, patch)
2013-01-11 18:06 UTC, Stef Walter
committed Details | Review
egg-armor: Handle mismatched but not truncated suffix line (1.54 KB, patch)
2013-01-11 20:49 UTC, Stef Walter
committed Details | Review

Description Colin Walters 2013-01-10 21:40:15 UTC
Created attachment 233185 [details] [review]
[PATCH] egg-armor: Fix memrchr() call with negative string length

I could not reproduce this issue reliably as is, but Karsten pointed
me to a variant of this bug that could be reproduced easily:
gnome-keyring import ~/.ssh/id_rsa

See https://bugzilla.redhat.com/show_bug.cgi?id=893162
---
 egg/egg-armor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Comment 1 Stef Walter 2013-01-11 17:21:47 UTC
This looks good. I'll try to add a test case for it.
Comment 2 Stef Walter 2013-01-11 18:06:16 UTC
Created attachment 233255 [details] [review]
Patch with tests and fixes bad practice for git master

egg-armor: Fix memrchr() call with negative string length

 * Initial patch by Gustavo Luiz Duarte <gustavold@linux.vnet.ibm.com>
 * The cause of this bug was reusing argument variables for other
   purposes in parsing functions when that didn't really make sense,
   so fix this as well.
 * Add tests that catch this issue.

See https://bugzilla.redhat.com/show_bug.cgi?id=893162
Comment 3 Colin Walters 2013-01-11 19:41:44 UTC
Review of attachment 233255 [details] [review]:

I didn't study the actual logic too closely, but it looks right in outline, and the addition of tests greatly increases confidence here.
Comment 4 Gustavo Luiz Duarte 2013-01-11 20:15:28 UTC
Review of attachment 233255 [details] [review]:

Looks great. And thanks Stef for adding the test case.

I'm just wondering if that '&&' on the following line shouldn't be a '||'.

if (ARMOR_SUFF_L > len && strncmp ((gchar*)at, ARMOR_SUFF, ARMOR_SUFF_L) != 0)
Comment 5 Stef Walter 2013-01-11 20:49:47 UTC
Created attachment 233260 [details] [review]
egg-armor: Handle mismatched but not truncated suffix line

 * Discovered by Gustavo Luiz Duarte <gustavold@linux.vnet.ibm.com>
Comment 6 Stef Walter 2013-01-13 08:39:58 UTC
Attachment 233260 [details] pushed as 583d9cc - egg-armor: Handle mismatched but not truncated suffix line
Comment 7 Stef Walter 2013-01-13 08:42:46 UTC
For distributors back-porting this fix: The original patch is an adequate and minimal fix for the issue.