php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #68277 passing NULL in place of GeoIP struct lead to segfault in libGeoIP
Submitted: 2014-10-21 14:48 UTC Modified: 2014-10-23 13:07 UTC
Votes:4
Avg. Score:4.5 ± 0.5
Reproduced:4 of 4 (100.0%)
Same Version:1 (25.0%)
Same OS:3 (75.0%)
From: slim at inbox dot lv Assigned:
Status: Open Package: geoip (PECL)
PHP Version: 5.4.34 OS: linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: slim at inbox dot lv
New email:
PHP Version: OS:

 

 [2014-10-21 14:48 UTC] slim at inbox dot lv
Description:
------------
in function geoip_record_by_name() after gi = GeoIP_open_type() `gi` must be checked for NULL before gir = GeoIP_record_by_name(gi, hostname);

libgeoip does not check gi and segfaults



Test script:
---------------
any broken /usr/share/GeoIP/GeoIPCity.dat file leads to segfault in libGeoIP

Expected result:
----------------
do not crash

Actual result:
--------------
segfault

Patches

patch-geoip-open-type (last revision 2014-10-23 15:22 UTC by phpbugs at joern dot heissler dot de)
geoip-1.1.0-city.patch (last revision 2014-10-23 13:08 UTC by slim at inbox dot lv)
patch-geoip_record_by_name (last revision 2014-10-23 02:47 UTC by phpbugs at joern dot heissler dot de)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-10-23 02:50 UTC] phpbugs at joern dot heissler dot de
Hi,
I submitted a patch. Please review it carefully for it's almost 5am *yawn*
 [2014-10-23 03:03 UTC] phpbugs at joern dot heissler dot de
Oh, and also see the GeoIP_db_avail function please. It checks only if a file exists, but these two database revisions share the same filename:

GeoIPDBFileName[GEOIP_CITY_EDITION_REV0] = _GeoIP_full_path_to(
    "GeoIPCity.dat");
GeoIPDBFileName[GEOIP_CITY_EDITION_REV1] = _GeoIP_full_path_to(
    "GeoIPCity.dat");

On my setup REV1 is said to be available, but apparently I'm using REV0 files. Trying to open my valid REV0 file as REV1 failed and caused a segfault too.
 [2014-10-23 13:07 UTC] slim at inbox dot lv
the patch is wrong as you see
Check my one.
BTW gi need to be checked for NULL before every GeoIP_*_by_name() function !
 [2014-10-23 15:20 UTC] phpbugs at joern dot heissler dot de
I don't see how my patch is wrong. Please explain.
It's certainly incomplete though, as it fixes only one occurrence of the bug. At a closer look, I think this bug has been repeated about a dozen times. I refactored the code a bit in a new patch.

Your patch is wrong and only works by accident:
If GeoIP_db_avail(GEOIP_CITY_EDITION_REV1) is true, you'll next GeoIP_open_type(GEOIP_CITY_EDITION_REV0, GEOIP_STANDARD).

These are different editions, you shouldn't check availability of one and open the other.
 [2014-10-27 13:05 UTC] slim at inbox dot lv
can You please push your patch to pecl-geoip and release next version ?
 [2014-11-20 20:53 UTC] anthon at piwik dot org
Previously reported in https://bugs.php.net/bug.php?id=67231. Please mark 67231 as a dupe since there appears to be more active discussion here. I'll comment on the proposed patches once I have a chance to review.
 [2015-04-17 02:50 UTC] goschwald at maxmind dot com
It would be great to see this issue fixed. It was reported upstream here:

https://github.com/maxmind/geoip-api-c/issues/59

GeoIP_open_type will return NULL if the open fails, which can happen even if GeoIP_db_avail returns true. The latter only checks that the file exists and does not check if it is the right type.
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Sun Sep 27 14:01:24 2020 UTC