From fc6c67ccf3f1819e90cfd0f10408a2724fbaff97 Mon Sep 17 00:00:00 2001 From: Samuel Tan Date: Mon, 18 Jul 2016 09:49:59 -0700 Subject: VenueNameElement: fix off-by-one enum bounds check Fix the off-by-one error in the conditionals that check whether the Venue Group and Venue Type codes in the ANQP element are in the "Reserved" range. BUG: 30169673 BUG: 29464811 TEST: Manually set up AP with Hotspot 2.0 support, broadcasting Venue Group value 0xc, and ensure that device does not crash when in range of this AP. Change-Id: I14adc3a919e19b67fc0f46bf09d0cffb88b5354e (cherry picked from commit 48ee5f1e1c6e2a2dc63e9cb84c42f532c8a6847a) --- service/java/com/android/server/wifi/anqp/VenueNameElement.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/service/java/com/android/server/wifi/anqp/VenueNameElement.java b/service/java/com/android/server/wifi/anqp/VenueNameElement.java index f0b27dd33..f944c4098 100644 --- a/service/java/com/android/server/wifi/anqp/VenueNameElement.java +++ b/service/java/com/android/server/wifi/anqp/VenueNameElement.java @@ -29,13 +29,13 @@ public class VenueNameElement extends ANQPElement { int group = payload.get() & Constants.BYTE_MASK; int type = payload.get() & Constants.BYTE_MASK; - if (group >= VenueGroup.values().length) { + if (group >= VenueGroup.Reserved.ordinal()) { mGroup = VenueGroup.Reserved; mType = VenueType.Reserved; } else { mGroup = VenueGroup.values()[group]; type += sGroupBases.get(mGroup); - if (type >= VenueType.values().length) { + if (type >= VenueType.Reserved.ordinal()) { mType = VenueType.Reserved; } else { mType = VenueType.values()[type]; @@ -82,7 +82,7 @@ public class VenueNameElement extends ANQPElement { UtilityMiscellaneous, Vehicular, Outdoor, - Reserved + Reserved // Note: this must be the last enum constant } public enum VenueType { @@ -164,7 +164,7 @@ public class VenueNameElement extends ANQPElement { BusStop, Kiosk, - Reserved + Reserved // Note: this must be the last enum constant } private static final VenueType[] PerGroup = -- cgit v1.2.3 From 47c5adba752a951243ee696ce5f1ee3f302a42ab Mon Sep 17 00:00:00 2001 From: Samuel Tan Date: Tue, 19 Jul 2016 17:35:57 -0700 Subject: ANQPFactory: catch all potential parsing errors The ANQP Element parsing code that parses untrusted data broadcasted by APs is currently untested, and might contain errors that will trigger exceptions that can crash the system service (e.g. null pointer exceptions). To contain this risk, catch all possible exceptions from the invoking ANQP element parsing code from ANQPFactory, and throw them again as ProtocolExceptions, which users of ANQPFactory already catch. BUG: 30230534 Change-Id: Icaba02c0e6739d94482cf4a5e704b59f8d4105b4 (cherry picked from commit 6154eb070b9f224a8daebf0a852d61f07d2c5cf3) --- .../com/android/server/wifi/anqp/ANQPFactory.java | 158 ++++++++++++--------- 1 file changed, 87 insertions(+), 71 deletions(-) diff --git a/service/java/com/android/server/wifi/anqp/ANQPFactory.java b/service/java/com/android/server/wifi/anqp/ANQPFactory.java index 55f631073..deca9c6c6 100644 --- a/service/java/com/android/server/wifi/anqp/ANQPFactory.java +++ b/service/java/com/android/server/wifi/anqp/ANQPFactory.java @@ -158,83 +158,99 @@ public class ANQPFactory { public static ANQPElement buildElement(ByteBuffer payload, Constants.ANQPElementType infoID, int length) throws ProtocolException { - ByteBuffer elementPayload = payload.duplicate().order(ByteOrder.LITTLE_ENDIAN); - payload.position(payload.position() + length); - elementPayload.limit(elementPayload.position() + length); - - switch (infoID) { - case ANQPCapabilityList: - return new CapabilityListElement(infoID, elementPayload); - case ANQPVenueName: - return new VenueNameElement(infoID, elementPayload); - case ANQPEmergencyNumber: - return new EmergencyNumberElement(infoID, elementPayload); - case ANQPNwkAuthType: - return new NetworkAuthenticationTypeElement(infoID, elementPayload); - case ANQPRoamingConsortium: - return new RoamingConsortiumElement(infoID, elementPayload); - case ANQPIPAddrAvailability: - return new IPAddressTypeAvailabilityElement(infoID, elementPayload); - case ANQPNAIRealm: - return new NAIRealmElement(infoID, elementPayload); - case ANQP3GPPNetwork: - return new ThreeGPPNetworkElement(infoID, elementPayload); - case ANQPGeoLoc: - return new GEOLocationElement(infoID, elementPayload); - case ANQPCivicLoc: - return new CivicLocationElement(infoID, elementPayload); - case ANQPLocURI: - return new GenericStringElement(infoID, elementPayload); - case ANQPDomName: - return new DomainNameElement(infoID, elementPayload); - case ANQPEmergencyAlert: - return new GenericStringElement(infoID, elementPayload); - case ANQPTDLSCap: - return new GenericBlobElement(infoID, elementPayload); - case ANQPEmergencyNAI: - return new GenericStringElement(infoID, elementPayload); - case ANQPNeighborReport: - return new GenericBlobElement(infoID, elementPayload); - case ANQPVendorSpec: - if (elementPayload.remaining() > 5) { - int oi = elementPayload.getInt(); - if (oi != Constants.HS20_PREFIX) { - return null; - } - int subType = elementPayload.get() & Constants.BYTE_MASK; - Constants.ANQPElementType hs20ID = Constants.mapHS20Element(subType); - if (hs20ID == null) { - throw new ProtocolException("Bad HS20 info ID: " + subType); - } - elementPayload.get(); // Skip the reserved octet - return buildHS20Element(hs20ID, elementPayload); - } else { + try { + ByteBuffer elementPayload = payload.duplicate().order(ByteOrder.LITTLE_ENDIAN); + payload.position(payload.position() + length); + elementPayload.limit(elementPayload.position() + length); + + switch (infoID) { + case ANQPCapabilityList: + return new CapabilityListElement(infoID, elementPayload); + case ANQPVenueName: + return new VenueNameElement(infoID, elementPayload); + case ANQPEmergencyNumber: + return new EmergencyNumberElement(infoID, elementPayload); + case ANQPNwkAuthType: + return new NetworkAuthenticationTypeElement(infoID, elementPayload); + case ANQPRoamingConsortium: + return new RoamingConsortiumElement(infoID, elementPayload); + case ANQPIPAddrAvailability: + return new IPAddressTypeAvailabilityElement(infoID, elementPayload); + case ANQPNAIRealm: + return new NAIRealmElement(infoID, elementPayload); + case ANQP3GPPNetwork: + return new ThreeGPPNetworkElement(infoID, elementPayload); + case ANQPGeoLoc: + return new GEOLocationElement(infoID, elementPayload); + case ANQPCivicLoc: + return new CivicLocationElement(infoID, elementPayload); + case ANQPLocURI: + return new GenericStringElement(infoID, elementPayload); + case ANQPDomName: + return new DomainNameElement(infoID, elementPayload); + case ANQPEmergencyAlert: + return new GenericStringElement(infoID, elementPayload); + case ANQPTDLSCap: return new GenericBlobElement(infoID, elementPayload); - } - default: - throw new ProtocolException("Unknown element ID: " + infoID); + case ANQPEmergencyNAI: + return new GenericStringElement(infoID, elementPayload); + case ANQPNeighborReport: + return new GenericBlobElement(infoID, elementPayload); + case ANQPVendorSpec: + if (elementPayload.remaining() > 5) { + int oi = elementPayload.getInt(); + if (oi != Constants.HS20_PREFIX) { + return null; + } + int subType = elementPayload.get() & Constants.BYTE_MASK; + Constants.ANQPElementType hs20ID = Constants.mapHS20Element(subType); + if (hs20ID == null) { + throw new ProtocolException("Bad HS20 info ID: " + subType); + } + elementPayload.get(); // Skip the reserved octet + return buildHS20Element(hs20ID, elementPayload); + } else { + return new GenericBlobElement(infoID, elementPayload); + } + default: + throw new ProtocolException("Unknown element ID: " + infoID); + } + } catch (ProtocolException e) { + throw e; + } catch (Exception e) { + // TODO: remove this catch-all for exceptions, once the element parsing code + // has been thoroughly unit tested. b/30562650 + throw new ProtocolException("Unknown parsing error", e); } } public static ANQPElement buildHS20Element(Constants.ANQPElementType infoID, ByteBuffer payload) throws ProtocolException { - switch (infoID) { - case HSCapabilityList: - return new HSCapabilityListElement(infoID, payload); - case HSFriendlyName: - return new HSFriendlyNameElement(infoID, payload); - case HSWANMetrics: - return new HSWanMetricsElement(infoID, payload); - case HSConnCapability: - return new HSConnectionCapabilityElement(infoID, payload); - case HSOperatingclass: - return new GenericBlobElement(infoID, payload); - case HSOSUProviders: - return new HSOsuProvidersElement(infoID, payload); - case HSIconFile: - return new HSIconFileElement(infoID, payload); - default: - return null; + try { + switch (infoID) { + case HSCapabilityList: + return new HSCapabilityListElement(infoID, payload); + case HSFriendlyName: + return new HSFriendlyNameElement(infoID, payload); + case HSWANMetrics: + return new HSWanMetricsElement(infoID, payload); + case HSConnCapability: + return new HSConnectionCapabilityElement(infoID, payload); + case HSOperatingclass: + return new GenericBlobElement(infoID, payload); + case HSOSUProviders: + return new HSOsuProvidersElement(infoID, payload); + case HSIconFile: + return new HSIconFileElement(infoID, payload); + default: + return null; + } + } catch (ProtocolException e) { + throw e; + } catch (Exception e) { + // TODO: remove this catch-all for exceptions, once the element parsing code + // has been thoroughly unit tested. b/30562650 + throw new ProtocolException("Unknown parsing error", e); } } } -- cgit v1.2.3