Skip to content

Commit e20b878

Browse files
COM API: return an error, not success, when the caller is not a server admin
Fifteen COM methods rejected an unauthorized caller with `return false`. These functions return HRESULT, where false is 0 - which is S_OK. Every one of them reported success for a call it had just refused. The consequences differ by method but none are benign: - InterfaceCache's five getters return before writing *pVal, so a caller without server-admin rights receives S_OK and reads whatever happened to be in the out-parameter. Uninitialized memory handed to any COM client. - InterfaceSettings::SetAdministratorPassword and five siblings skip the write and report success, so a caller is told the administrator password changed when it did not. - InterfaceMessageIndexing's four methods behave the same way. This is the file's own convention being broken rather than a design choice: InterfaceSettings.cpp already returns authentication_->GetAccessDenied() in twelve other places, and each of the fifteen sites calls GetAccessDenied() one line above for the null-config check. They now do the same for the authorization check. Found by running CodeQL's C++ suite locally. The workflow analyses csharp only, so 4.65 MB of network-facing code - the largest language in the repository and the entire protocol surface - had never been scanned. The scan returned 16 high-severity findings, 15 of them these. Re-running it after the fix returns 1: cpp/incorrect-string-type-conversion in FileUtilities.cpp, which is a false positive - the cast binds before the +1, so it skips a UTF-16 BOM rather than a byte. Builds clean under /WX. Note the regression suite could not be used to validate this: ESET's mail proxying rewrites the IMAP literal continuation on this machine and fails 536 of 1026 tests regardless of the change.
1 parent 2dbf606 commit e20b878

3 files changed

Lines changed: 15 additions & 15 deletions

File tree

hmailserver/source/Server/COM/InterfaceCache.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ InterfaceCache::get_DomainHitRate(long *pVal)
114114
return GetAccessDenied();
115115

116116
if (!GetIsServerAdmin())
117-
return false;
117+
return GetAccessDenied();
118118

119119
*pVal = HM::Cache<HM::Domain>::Instance()->GetHitRate();
120120

@@ -223,7 +223,7 @@ InterfaceCache::get_AccountHitRate(long *pVal)
223223
return GetAccessDenied();
224224

225225
if (!GetIsServerAdmin())
226-
return false;
226+
return GetAccessDenied();
227227

228228
*pVal = HM::Cache<HM::Account>::Instance()->GetHitRate();
229229
return S_OK;
@@ -331,7 +331,7 @@ InterfaceCache::get_AliasHitRate(long *pVal)
331331
return GetAccessDenied();
332332

333333
if (!GetIsServerAdmin())
334-
return false;
334+
return GetAccessDenied();
335335

336336
*pVal = HM::Cache<HM::Alias>::Instance()->GetHitRate();
337337
return S_OK;
@@ -438,7 +438,7 @@ InterfaceCache::get_DistributionListHitRate(long *pVal)
438438
return GetAccessDenied();
439439

440440
if (!GetIsServerAdmin())
441-
return false;
441+
return GetAccessDenied();
442442

443443
*pVal = HM::Cache<HM::DistributionList>::Instance()->GetHitRate();
444444
return S_OK;
@@ -510,7 +510,7 @@ InterfaceCache::Clear()
510510
return GetAccessDenied();
511511

512512
if (!GetIsServerAdmin())
513-
return false;
513+
return GetAccessDenied();
514514

515515
HM::Cache<HM::Account>::Instance()->Clear();
516516
HM::Cache<HM::Domain>::Instance()->Clear();

hmailserver/source/Server/COM/InterfaceMessageIndexing.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ STDMETHODIMP InterfaceMessageIndexing::get_TotalMessageCount(long *pVal)
7070
return GetAccessDenied();
7171

7272
if (!GetIsServerAdmin())
73-
return false;
73+
return GetAccessDenied();
7474

7575
*pVal = HM::PersistentMessage::GetTotalMessageCountDelivered();
7676

@@ -87,7 +87,7 @@ STDMETHODIMP InterfaceMessageIndexing::get_TotalIndexedCount(long *pVal)
8787
try
8888
{
8989
if (!GetIsServerAdmin())
90-
return false;
90+
return GetAccessDenied();
9191

9292
HM::PersistentMessageMetaData md;
9393
*pVal = md.GetTotalMessageCount();
@@ -105,7 +105,7 @@ STDMETHODIMP InterfaceMessageIndexing::Clear()
105105
try
106106
{
107107
if (!GetIsServerAdmin())
108-
return false;
108+
return GetAccessDenied();
109109

110110
HM::PersistentMessageMetaData md;
111111

@@ -124,7 +124,7 @@ STDMETHODIMP InterfaceMessageIndexing::Index()
124124
try
125125
{
126126
if (!GetIsServerAdmin())
127-
return false;
127+
return GetAccessDenied();
128128

129129
HM::MessageIndexer::Instance()->IndexNow();
130130

hmailserver/source/Server/COM/InterfaceSettings.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1020,7 +1020,7 @@ STDMETHODIMP InterfaceSettings::SetAdministratorPassword(BSTR newVal)
10201020
return GetAccessDenied();
10211021

10221022
if (!GetIsServerAdmin())
1023-
return false;
1023+
return GetAccessDenied();
10241024

10251025

10261026
ini_file_settings_->SetAdministratorPassword(newVal);
@@ -1218,7 +1218,7 @@ STDMETHODIMP InterfaceSettings::get_SSLCertificates(IInterfaceSSLCertificates **
12181218
return GetAccessDenied();
12191219

12201220
if (!GetIsServerAdmin())
1221-
return false;
1221+
return GetAccessDenied();
12221222

12231223
std::shared_ptr<HM::SSLCertificates> pSSLCertificates = HM::Configuration::Instance()->GetSSLCertificates();
12241224

@@ -1871,7 +1871,7 @@ STDMETHODIMP InterfaceSettings::get_PublicFolders(IInterfaceIMAPFolders **pVal)
18711871
return GetAccessDenied();
18721872

18731873
if (!GetIsServerAdmin())
1874-
return false;
1874+
return GetAccessDenied();
18751875

18761876
std::shared_ptr<HM::IMAPFolders> pIMAPFolders = HM::Configuration::Instance()->GetIMAPConfiguration()->GetPublicFolders();
18771877

@@ -1898,7 +1898,7 @@ STDMETHODIMP InterfaceSettings::get_PublicFolderDiskName(BSTR *pVal)
18981898
return GetAccessDenied();
18991899

19001900
if (!GetIsServerAdmin())
1901-
return false;
1901+
return GetAccessDenied();
19021902

19031903
*pVal = config_->GetIMAPConfiguration()->GetPublicFolderDiskName().AllocSysString();
19041904
return S_OK;
@@ -1917,7 +1917,7 @@ STDMETHODIMP InterfaceSettings::get_Groups(IInterfaceGroups **pVal)
19171917
return GetAccessDenied();
19181918

19191919
if (!GetIsServerAdmin())
1920-
return false;
1920+
return GetAccessDenied();
19211921

19221922
CComObject<InterfaceGroups>* pItem = new CComObject<InterfaceGroups>();
19231923
pItem->SetAuthentication(authentication_);
@@ -2141,7 +2141,7 @@ STDMETHODIMP InterfaceSettings::ClearLogonFailureList()
21412141
return GetAccessDenied();
21422142

21432143
if (!GetIsServerAdmin())
2144-
return false;
2144+
return GetAccessDenied();
21452145

21462146
config_->ClearOldLogonFailures();
21472147

0 commit comments

Comments
 (0)