Skip to content

Commit 1d3b4ab

Browse files
randombitrandombit
authored
Merge pull request #5673 from randombit/jack/sql-table-checks
Validate SQL table names
2 parents 695e153 + 4610aac commit 1d3b4ab

5 files changed

Lines changed: 76 additions & 35 deletions

File tree

‎src/lib/psk_db/psk_db_sql.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include <botan/psk_db.h>
88

9+
#include <botan/assert.h>
910
#include <botan/database.h>
1011

1112
namespace Botan {
@@ -15,6 +16,7 @@ Encrypted_PSK_Database_SQL::Encrypted_PSK_Database_SQL(const secure_vector<uint8
1516
std::string_view table_name) :
1617
Encrypted_PSK_Database(master_key), m_db(std::move(db)), m_table_name(table_name) {
1718
using DB = SQL_Database;
19+
BOTAN_ARG_CHECK(m_db->is_valid_table_name(m_table_name), "Provided table name is not valid for this database");
1820
m_db->create_table(DB::Table_Schema(m_table_name,
1921
{
2022
DB::Column("psk_name", DB::Column_Type::String).primary_key(),

‎src/lib/utils/database.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,25 @@
66

77
#include <botan/database.h>
88

9+
#include <botan/internal/charset.h>
910
#include <string>
1011

1112
namespace Botan {
1213

14+
bool SQL_Database::is_valid_table_name(std::string_view table) const {
15+
if(table.empty()) {
16+
return false;
17+
}
18+
19+
constexpr auto valid_table_name_char = CharacterValidityTable::alpha_numeric_plus("_");
20+
for(const char c : table) {
21+
if(!valid_table_name_char(c)) {
22+
return false;
23+
}
24+
}
25+
return true;
26+
}
27+
1328
std::shared_ptr<SQL_Database::Statement> SQL_Database::select(std::string_view columns,
1429
std::string_view table,
1530
std::string_view where,

‎src/lib/utils/database.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,13 @@ class BOTAN_PUBLIC_API(2, 0) SQL_Database /* NOLINT(*-special-member-functions)
171171

172172
virtual bool is_threadsafe() const { return false; }
173173

174+
/**
175+
* Return true if the given name seems to be valid as the name for a table
176+
*
177+
* Default implementation accepts non-empty [a-zA-Z0-9_]
178+
*/
179+
virtual bool is_valid_table_name(std::string_view table) const;
180+
174181
virtual ~SQL_Database() = default;
175182
};
176183

‎src/lib/x509/certstor_sql/certstor_sql.cpp‎

Lines changed: 47 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,35 @@
1010

1111
#include <botan/asn1_obj.h>
1212
#include <botan/asn1_time.h>
13+
#include <botan/assert.h>
1314
#include <botan/ber_dec.h>
1415
#include <botan/data_src.h>
1516
#include <botan/pk_keys.h>
1617
#include <botan/pkcs8.h>
1718
#include <botan/pkix_types.h>
19+
#include <botan/internal/fmt.h>
1820

1921
namespace Botan {
2022

2123
Certificate_Store_In_SQL::Certificate_Store_In_SQL(std::shared_ptr<SQL_Database> db,
2224
std::string_view passwd,
2325
RandomNumberGenerator& rng,
2426
std::string_view table_prefix) :
25-
m_rng(rng), m_database(std::move(db)), m_prefix(table_prefix), m_password(passwd) {
27+
m_rng(rng),
28+
m_database(std::move(db)),
29+
m_db_cert_table(fmt("{}certificates", table_prefix)),
30+
m_db_keys_table(fmt("{}keys", table_prefix)),
31+
m_db_crls_table(fmt("{}revoked", table_prefix)),
32+
m_password(passwd) {
2633
using DB = SQL_Database;
2734
const auto blob = DB::Column_Type::Blob;
2835
const auto integer = DB::Column_Type::Integer;
2936

30-
m_database->create_table(DB::Table_Schema(m_prefix + "certificates",
37+
BOTAN_ARG_CHECK(m_database->is_valid_table_name(m_db_cert_table), "Invalid table name");
38+
BOTAN_ARG_CHECK(m_database->is_valid_table_name(m_db_keys_table), "Invalid table name");
39+
BOTAN_ARG_CHECK(m_database->is_valid_table_name(m_db_crls_table), "Invalid table name");
40+
41+
m_database->create_table(DB::Table_Schema(m_db_cert_table,
3142
{
3243
DB::Column("fingerprint", blob).primary_key(),
3344
DB::Column("subject_dn", blob),
@@ -37,14 +48,14 @@ Certificate_Store_In_SQL::Certificate_Store_In_SQL(std::shared_ptr<SQL_Database>
3748
})
3849
.if_not_exists());
3950

40-
m_database->create_table(DB::Table_Schema(m_prefix + "keys",
51+
m_database->create_table(DB::Table_Schema(m_db_keys_table,
4152
{
4253
DB::Column("fingerprint", blob).primary_key(),
4354
DB::Column("key", blob).not_null(),
4455
})
4556
.if_not_exists());
4657

47-
m_database->create_table(DB::Table_Schema(m_prefix + "revoked",
58+
m_database->create_table(DB::Table_Schema(m_db_crls_table,
4859
{
4960
DB::Column("fingerprint", blob).primary_key(),
5061
DB::Column("reason", integer).not_null(),
@@ -61,11 +72,11 @@ std::optional<X509_Certificate> Certificate_Store_In_SQL::find_cert(const X509_D
6172
const std::vector<uint8_t> dn_encoding = subject_dn.BER_encode();
6273

6374
if(key_id.empty()) {
64-
stmt = m_database->select("certificate", m_prefix + "certificates", "subject_dn = ?1", 1);
75+
stmt = m_database->select("certificate", m_db_cert_table, "subject_dn = ?1", 1);
6576
stmt->bind(1, dn_encoding);
6677
} else {
67-
stmt = m_database->select(
68-
"certificate", m_prefix + "certificates", "subject_dn = ?1 AND (key_id IS NULL OR key_id = ?2)", 1);
78+
stmt =
79+
m_database->select("certificate", m_db_cert_table, "subject_dn = ?1 AND (key_id IS NULL OR key_id = ?2)", 1);
6980
stmt->bind(1, dn_encoding);
7081
stmt->bind(2, key_id);
7182
}
@@ -86,11 +97,10 @@ std::vector<X509_Certificate> Certificate_Store_In_SQL::find_all_certs(const X50
8697
const std::vector<uint8_t> dn_encoding = subject_dn.BER_encode();
8798

8899
if(key_id.empty()) {
89-
stmt = m_database->select("certificate", m_prefix + "certificates", "subject_dn = ?1");
100+
stmt = m_database->select("certificate", m_db_cert_table, "subject_dn = ?1");
90101
stmt->bind(1, dn_encoding);
91102
} else {
92-
stmt = m_database->select(
93-
"certificate", m_prefix + "certificates", "subject_dn = ?1 AND (key_id IS NULL OR key_id = ?2)");
103+
stmt = m_database->select("certificate", m_db_cert_table, "subject_dn = ?1 AND (key_id IS NULL OR key_id = ?2)");
94104
stmt->bind(1, dn_encoding);
95105
stmt->bind(2, key_id);
96106
}
@@ -131,7 +141,7 @@ std::optional<X509_CRL> Certificate_Store_In_SQL::find_crl_for(const X509_Certif
131141

132142
std::vector<X509_DN> Certificate_Store_In_SQL::all_subjects() const {
133143
std::vector<X509_DN> ret;
134-
auto stmt = m_database->select("subject_dn", m_prefix + "certificates");
144+
auto stmt = m_database->select("subject_dn", m_db_cert_table);
135145

136146
while(stmt->step()) {
137147
BER_Decoder dec(stmt->get_blob(0), BER_Decoder::Limits::DER());
@@ -149,8 +159,8 @@ bool Certificate_Store_In_SQL::insert_cert(const X509_Certificate& cert) {
149159
const std::vector<uint8_t> dn_encoding = cert.subject_dn().BER_encode();
150160
const std::vector<uint8_t> cert_encoding = cert.BER_encode();
151161

152-
auto stmt = m_database->upsert(m_prefix + "certificates",
153-
{"fingerprint", "subject_dn", "key_id", "priv_fingerprint", "certificate"});
162+
auto stmt =
163+
m_database->upsert(m_db_cert_table, {"fingerprint", "subject_dn", "key_id", "priv_fingerprint", "certificate"});
154164

155165
stmt->bind(1, cert.fingerprint("SHA-256"));
156166
stmt->bind(2, dn_encoding);
@@ -163,7 +173,7 @@ bool Certificate_Store_In_SQL::insert_cert(const X509_Certificate& cert) {
163173
}
164174

165175
bool Certificate_Store_In_SQL::contains(const X509_Certificate& cert) const {
166-
auto stmt = m_database->select("1", m_prefix + "certificates", "fingerprint = ?1");
176+
auto stmt = m_database->select("1", m_db_cert_table, "fingerprint = ?1");
167177
stmt->bind(1, cert.fingerprint("SHA-256"));
168178
return stmt->step();
169179
}
@@ -173,7 +183,7 @@ bool Certificate_Store_In_SQL::remove_cert(const X509_Certificate& cert) {
173183
return false;
174184
}
175185

176-
auto stmt = m_database->new_statement("DELETE FROM " + m_prefix + "certificates WHERE fingerprint = ?1");
186+
auto stmt = m_database->new_statement(fmt("DELETE FROM {} WHERE fingerprint = ?1", m_db_cert_table));
177187

178188
stmt->bind(1, cert.fingerprint("SHA-256"));
179189
stmt->spin();
@@ -183,13 +193,14 @@ bool Certificate_Store_In_SQL::remove_cert(const X509_Certificate& cert) {
183193

184194
// Private key handling
185195
std::shared_ptr<const Private_Key> Certificate_Store_In_SQL::find_key(const X509_Certificate& cert) const {
186-
auto stmt = m_database->new_statement("SELECT key FROM " + m_prefix +
187-
"keys "
188-
"JOIN " +
189-
m_prefix + "certificates ON " + m_prefix + "keys.fingerprint = " + m_prefix +
190-
"certificates.priv_fingerprint "
191-
"WHERE " +
192-
m_prefix + "certificates.fingerprint = ?1");
196+
auto stmt =
197+
m_database->new_statement(fmt("SELECT key FROM {} JOIN {} ON {}.fingerprint = {}.priv_fingerprint "
198+
"WHERE {}.fingerprint = ?1",
199+
m_db_keys_table,
200+
m_db_cert_table,
201+
m_db_keys_table,
202+
m_db_cert_table,
203+
m_db_cert_table));
193204
stmt->bind(1, cert.fingerprint("SHA-256"));
194205

195206
std::shared_ptr<const Private_Key> key;
@@ -203,7 +214,7 @@ std::shared_ptr<const Private_Key> Certificate_Store_In_SQL::find_key(const X509
203214

204215
std::vector<X509_Certificate> Certificate_Store_In_SQL::find_certs_for_key(const Private_Key& key) const {
205216
auto fprint = key.fingerprint_private("SHA-256");
206-
auto stmt = m_database->select("certificate", m_prefix + "certificates", "priv_fingerprint = ?1");
217+
auto stmt = m_database->select("certificate", m_db_cert_table, "priv_fingerprint = ?1");
207218

208219
stmt->bind(1, fprint);
209220

@@ -225,14 +236,14 @@ bool Certificate_Store_In_SQL::insert_key(const X509_Certificate& cert, const Pr
225236
auto pkcs8 = PKCS8::BER_encode(key, m_rng, m_password);
226237
auto fprint = key.fingerprint_private("SHA-256");
227238

228-
auto stmt1 = m_database->upsert(m_prefix + "keys", {"fingerprint", "key"});
239+
auto stmt1 = m_database->upsert(m_db_keys_table, {"fingerprint", "key"});
229240

230241
stmt1->bind(1, fprint);
231242
stmt1->bind(2, pkcs8.data(), pkcs8.size());
232243
stmt1->spin();
233244

234245
auto stmt2 =
235-
m_database->new_statement("UPDATE " + m_prefix + "certificates SET priv_fingerprint = ?1 WHERE fingerprint = ?2");
246+
m_database->new_statement(fmt("UPDATE {} SET priv_fingerprint = ?1 WHERE fingerprint = ?2", m_db_cert_table));
236247

237248
stmt2->bind(1, fprint);
238249
stmt2->bind(2, cert.fingerprint("SHA-256"));
@@ -243,7 +254,7 @@ bool Certificate_Store_In_SQL::insert_key(const X509_Certificate& cert, const Pr
243254

244255
void Certificate_Store_In_SQL::remove_key(const Private_Key& key) {
245256
auto fprint = key.fingerprint_private("SHA-256");
246-
auto stmt = m_database->new_statement("DELETE FROM " + m_prefix + "keys WHERE fingerprint = ?1");
257+
auto stmt = m_database->new_statement(fmt("DELETE FROM {} WHERE fingerprint = ?1", m_db_keys_table));
247258

248259
stmt->bind(1, fprint);
249260
stmt->spin();
@@ -254,7 +265,7 @@ void Certificate_Store_In_SQL::revoke_cert(const X509_Certificate& cert, CRL_Cod
254265
// TODO(Botan4) require that time be valid
255266
insert_cert(cert);
256267

257-
auto stmt1 = m_database->upsert(m_prefix + "revoked", {"fingerprint", "reason", "time"});
268+
auto stmt1 = m_database->upsert(m_db_crls_table, {"fingerprint", "reason", "time"});
258269

259270
stmt1->bind(1, cert.fingerprint("SHA-256"));
260271
stmt1->bind(2, static_cast<uint32_t>(code));
@@ -272,7 +283,7 @@ void Certificate_Store_In_SQL::revoke_cert(const X509_Certificate& cert, CRL_Cod
272283
void Certificate_Store_In_SQL::revoke_cert(const X509_Certificate& cert, CRL_Code code) {
273284
insert_cert(cert);
274285

275-
auto stmt1 = m_database->upsert(m_prefix + "revoked", {"fingerprint", "reason", "time"});
286+
auto stmt1 = m_database->upsert(m_db_crls_table, {"fingerprint", "reason", "time"});
276287

277288
stmt1->bind(1, cert.fingerprint("SHA-256"));
278289
stmt1->bind(2, static_cast<uint32_t>(code));
@@ -282,18 +293,20 @@ void Certificate_Store_In_SQL::revoke_cert(const X509_Certificate& cert, CRL_Cod
282293
}
283294

284295
void Certificate_Store_In_SQL::affirm_cert(const X509_Certificate& cert) {
285-
auto stmt = m_database->new_statement("DELETE FROM " + m_prefix + "revoked WHERE fingerprint = ?1");
296+
auto stmt = m_database->new_statement(fmt("DELETE FROM {} WHERE fingerprint = ?1", m_db_crls_table));
286297

287298
stmt->bind(1, cert.fingerprint("SHA-256"));
288299
stmt->spin();
289300
}
290301

291302
std::vector<X509_CRL> Certificate_Store_In_SQL::generate_crls() const {
292-
auto stmt = m_database->new_statement("SELECT certificate,reason,time FROM " + m_prefix +
293-
"revoked "
294-
"JOIN " +
295-
m_prefix + "certificates ON " + m_prefix +
296-
"certificates.fingerprint = " + m_prefix + "revoked.fingerprint");
303+
auto stmt =
304+
m_database->new_statement(fmt("SELECT certificate,reason,time FROM {} JOIN {} ON {}.fingerprint = "
305+
"{}.fingerprint",
306+
m_db_crls_table,
307+
m_db_cert_table,
308+
m_db_cert_table,
309+
m_db_crls_table));
297310

298311
std::map<X509_DN, std::vector<CRL_Entry>> crls;
299312
while(stmt->step()) {

‎src/lib/x509/certstor_sql/certstor_sql.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,11 @@ class BOTAN_PUBLIC_API(2, 0) Certificate_Store_In_SQL : public Certificate_Store
113113
private:
114114
RandomNumberGenerator& m_rng;
115115
std::shared_ptr<SQL_Database> m_database;
116-
std::string m_prefix;
116+
117+
std::string m_db_cert_table;
118+
std::string m_db_keys_table;
119+
std::string m_db_crls_table;
120+
117121
std::string m_password;
118122
};
119123

0 commit comments

Comments
 (0)