Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 827b12f9 authored by Ken Chen's avatar Ken Chen Committed by Automerger Merge Worker
Browse files

Allow only AID_ROOT to inject certificate am: 26dc2b0a am: 66dfc76d 

Original change: https://android-review.googlesource.com/c/platform/packages/modules/DnsResolver/+/1339796

Change-Id: If54e001e6538b5c6764bb41d52756d7ad4583238
parents 9af96f85 66dfc76d

DnsResolverService.cpp

+4 −1
Original line number Diff line number Diff line
@@ -178,8 +178,11 @@ binder_status_t DnsResolverService::dump(int fd, const char** args, uint32_t num 
    // Locking happens in PrivateDnsConfiguration and res_* functions.

    ENFORCE_INTERNAL_PERMISSIONS();



    // TODO@: Switch to selinux based permission check if AIBinder_getCallingSid and

    //        AIBinder_setRequestingSid can be supported by libbinder_dnk (b/159135973).

    uid_t uid = AIBinder_getCallingUid();

    if (resolverParams.caCertificate.size() != 0 && uid == AID_SYSTEM) {

    // CAUTION: caCertificate should NOT be used except for internal testing.

    if (resolverParams.caCertificate.size() != 0 && uid != AID_ROOT) {

        auto err = StringPrintf("UID %d is not authorized to set a non-empty CA certificate", uid);

        return ::ndk::ScopedAStatus(AStatus_fromExceptionCodeWithMessage(EX_SECURITY, err.c_str()));

    }

DnsTlsSocket.cpp

+2 −2
Original line number Diff line number Diff line
@@ -148,8 +148,8 @@ bool DnsTlsSocket::initialize() { 
    //

    // For discussion of alternative, sustainable approaches see b/71909242.

    if (!mServer.certificate.empty()) {

        // Inject test CA certs from ResolverParamsParcel.caCertificate for internal testing.

        // This is only allowed by DnsResolverService if the caller is not AID_SYSTEM

        // Inject test CA certs from ResolverParamsParcel.caCertificate for INTERNAL TESTING ONLY.

        // This is only allowed by DnsResolverService if the caller is AID_ROOT.

        LOG(WARNING) << "Setting test CA certificate. This should never happen in production code.";

        if (!setTestCaCertificate()) {

            LOG(ERROR) << "Failed to set test CA certificate";

tests/resolv_integration_test.cpp

+12 −0
Original line number Diff line number Diff line
@@ -4591,6 +4591,18 @@ TEST_F(ResolverTest, RepeatedSetup_KeepChangingPrivateDnsServers) { 
    EXPECT_FALSE(hasUncaughtPrivateDnsValidation(addr2));

}



TEST_F(ResolverTest, PermissionCheckOnCertificateInjection) {

    ResolverParamsParcel parcel = DnsResponderClient::GetDefaultResolverParamsParcel();

    parcel.caCertificate = kCaCert;

    ASSERT_TRUE(mDnsClient.resolvService()->setResolverConfiguration(parcel).isOk());



    for (const uid_t uid : {AID_SYSTEM, TEST_UID}) {

        ScopedChangeUID scopedChangeUID(uid);

        auto status = mDnsClient.resolvService()->setResolverConfiguration(parcel);

        EXPECT_EQ(status.getExceptionCode(), EX_SECURITY);

    }

}



// Parameterized tests.

// TODO: Merge the existing tests as parameterized test if possible.

// TODO: Perhaps move parameterized tests to an independent file.

tests/resolv_test_utils.h

+12 −0
Original line number Diff line number Diff line
@@ -62,6 +62,18 @@ class ScopeBlockedUIDRule { 
    const uid_t mSavedUid;

};



class ScopedChangeUID {

  public:

    ScopedChangeUID(uid_t testUid) : mTestUid(testUid), mSavedUid(getuid()) {

        EXPECT_TRUE(seteuid(mTestUid) == 0);

    };

    ~ScopedChangeUID() { EXPECT_TRUE(seteuid(mSavedUid) == 0); }



  private:

    const uid_t mTestUid;

    const uid_t mSavedUid;

};



struct DnsRecord {

    std::string host_name;  // host name

    ns_type type;           // record type